[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.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@256610 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/AST/ASTContext.h b/include/clang/AST/ASTContext.h
index b24e7652312e228afd9294d49cd81ae680494af3..44b5e3989b2df7009892cf2570963c5ae261bfb0 100644 (file)
--- a/include/clang/AST/ASTContext.h
+++ b/include/clang/AST/ASTContext.h
@@ -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,

diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
index b344b0687f2a250195ff823b087dc2735115c86e..508124a2a6eb45756d067230ca44308af8393bbc 100644 (file)
--- a/lib/AST/ASTContext.cpp
+++ b/lib/AST/ASTContext.cpp
@@ -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