r256610 - [ptr-traits] Switch from a really wasteful SmallDenseMap of

[Chandler Carruth via cfe-commits]

Author: chandlerc
Date: Tue Dec 29 21:00:23 2015
New Revision: 256610

URL: http://llvm.org/viewvc/llvm-project?rev=256610&view=rev
Log:
[ptr-traits] Switch from a really wasteful SmallDenseMap of
SmallVector<.., 16> (16!!!!) objects to a simple SmallVector of pairs.

This no longer de-duplicates the common function pointers used during
deallocation, but this doesn't really seem worth the complexity and
overhead of managing the map-of-vectors. Notably, there is no reason to
assume that functions have the 4-byte alignment that DenseMap relies on,
and indeed this prevents checking the alignment of the DenseMap keys
because we can't even meaningfully query the alignment of functions
using our existing alignment tools.

Generally, function pointers don't seem like a great idea for keys in
a DenseMap. =]

I chatted with Richard Smith about this a bit as well and have written
down a FIXME because this *does* waste some memory and in general seems
a very clumsy memory management strategy. He would like to see a more
fundamental fix eventually here that tries to build a better pattern.

Modified:
    cfe/trunk/include/clang/AST/ASTContext.h
    cfe/trunk/lib/AST/ASTContext.cpp

Modified: cfe/trunk/include/clang/AST/ASTContext.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=256610&r1=256609&r2=256610&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/ASTContext.h (original)
+++ cfe/trunk/include/clang/AST/ASTContext.h Tue Dec 29 21:00:23 2015
@@ -2514,9 +2514,15 @@ private:
 
   /// \brief A set of deallocations that should be performed when the
   /// ASTContext is destroyed.
-  typedef llvm::SmallDenseMap<void(*)(void*), llvm::SmallVector<void*, 16> >
-    DeallocationMap;
-  DeallocationMap Deallocations;
+  // FIXME: We really should have a better mechanism in the ASTContext to
+  // manage running destructors for types which do variable sized allocation
+  // within the AST. In some places we thread the AST bump pointer allocator
+  // into the datastructures which avoids this mess during deallocation but is
+  // wasteful of memory, and here we require a lot of error prone book keeping
+  // in order to track and run destructors while we're tearing things down.
+  typedef llvm::SmallVector<std::pair<void (*)(void *), void *>, 16>
+      DeallocationFunctionsAndArguments;
+  DeallocationFunctionsAndArguments Deallocations;
 
   // FIXME: This currently contains the set of StoredDeclMaps used
   // by DeclContext objects.  This probably should not be in ASTContext,

Modified: cfe/trunk/lib/AST/ASTContext.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=256610&r1=256609&r2=256610&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ASTContext.cpp (original)
+++ cfe/trunk/lib/AST/ASTContext.cpp Tue Dec 29 21:00:23 2015
@@ -760,10 +760,8 @@ ASTContext::~ASTContext() {
   ReleaseDeclContextMaps();
 
   // Call all of the deallocation functions on all of their targets.
-  for (DeallocationMap::const_iterator I = Deallocations.begin(),
-           E = Deallocations.end(); I != E; ++I)
-    for (unsigned J = 0, N = I->second.size(); J != N; ++J)
-      (I->first)((I->second)[J]);
+  for (auto &Pair : Deallocations)
+    (Pair.first)(Pair.second);
 
   // ASTRecordLayout objects in ASTRecordLayouts must always be destroyed
   // because they can contain DenseMaps.
@@ -812,7 +810,7 @@ void ASTContext::ReleaseParentMapEntries
 }
 
 void ASTContext::AddDeallocation(void (*Callback)(void*), void *Data) {
-  Deallocations[Callback].push_back(Data);
+  Deallocations.push_back({Callback, Data});
 }
 
 void