[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

[Philip Reames]

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.

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