[llvm] r204204 - When destroying a StringMap, just iterate over the map and destroy the contained elements. Don't reset them back to 0 as their values aren't needed any more. This results in ~StringMap() being mostly empty for POD types in BumpPtrAllocators

[David Blaikie]

On Wed, Mar 19, 2014 at 9:47 AM, Philip Reames
<listmail at philipreames.com> wrote:
> Is there a good performance justification for this change?  From what I can
> tell, you're removing one assignment per Bucket + 2 assignments.  This seems
> hardly worth the code duplication.
>
> Also, having destructed pointers be null is useful for catching
> use-after-free bugs.  I'm reluctant to give that up.
>
> Unless you can justify this change from a performance standpoint, I'd
> request that you revert it.
>
> p.s. If there was a review for this, sorry for not catching it.

Refer to the thread entitled "[PATCH] Make StringMap aware of POD types".

>
> Philip
>
>
> On 03/18/2014 05:23 PM, Pete Cooper wrote:
>>
>> Author: pete
>> Date: Tue Mar 18 19:23:30 2014
>> New Revision: 204204
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=204204&view=rev
>> Log:
>> When destroying a StringMap, just iterate over the map and destroy the
>> contained elements.  Don't reset them back to 0 as their values aren't
>> needed any more.  This results in ~StringMap() being mostly empty for POD
>> types in BumpPtrAllocators
>>
>> Modified:
>>      llvm/trunk/include/llvm/ADT/StringMap.h
>>
>> Modified: llvm/trunk/include/llvm/ADT/StringMap.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringMap.h?rev=204204&r1=204203&r2=204204&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ADT/StringMap.h (original)
>> +++ llvm/trunk/include/llvm/ADT/StringMap.h Tue Mar 18 19:23:30 2014
>> @@ -387,7 +387,17 @@ public:
>>     }
>>       ~StringMap() {
>> -    clear();
>> +    // Delete all the elements in the map, but don't reset the elements
>> +    // to default values.  This is a copy of clear(), but avoids
>> unnecessary
>> +    // work not required in the destructor.
>> +    if (!empty()) {
>> +      for (unsigned I = 0, E = NumBuckets; I != E; ++I) {
>> +        StringMapEntryBase *Bucket = TheTable[I];
>> +        if (Bucket && Bucket != getTombstoneVal()) {
>> +          static_cast<MapEntryTy*>(Bucket)->Destroy(Allocator);
>> +        }
>> +      }
>> +    }
>>       free(TheTable);
>>     }
>>   };
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits