r209297 - Make the parent-map use significantly less memory.

[Manuel Klimek]

On Wed, May 21, 2014 at 6:10 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> You might want to bump up the size of the SmallVector too. Right now
> you're dynamically allocating a SmallVector<T, 1>, and immediately putting
> two things into it. Alternately, you could use a std::vector, which has a
> smaller sizeof itself.
>

I tried the various combinations, and it didn't make a difference, so I
went for making the code the simplest, which was reusing the ParentVector
typedef.
If we change the ParentVector to a SmallVector<T, 2> we'll also use that
for getParents() which mostly returns a single element.


>
> Jordan
>
>
> On May 21, 2014, at 6:28 , Manuel Klimek <klimek at google.com> wrote:
>
> > Author: klimek
> > Date: Wed May 21 08:28:59 2014
> > New Revision: 209297
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=209297&view=rev
> > Log:
> > Make the parent-map use significantly less memory.
> >
> > On test files I ran this on, memory consumption overall went down from
> > 2.5G to 2G, without performance regressions.
> > I also investigated making DynTypedNode by itself smaller (by pulling
> > out pointers for everything that doesn't fit in 8 bytes). This led to
> > another 200-300MB saved, but also introduced a significant regression in
> > performance due to the memory management overhead.
> >
> > 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=209297&r1=209296&r2=209297&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/include/clang/AST/ASTContext.h (original)
> > +++ cfe/trunk/include/clang/AST/ASTContext.h Wed May 21 08:28:59 2014
> > @@ -424,7 +424,9 @@ public:
> >   typedef llvm::SmallVector<ast_type_traits::DynTypedNode, 1>
> ParentVector;
> >
> >   /// \brief Maps from a node to its parents.
> > -  typedef llvm::DenseMap<const void *, ParentVector> ParentMap;
> > +  typedef llvm::DenseMap<const void *,
> > +
> llvm::PointerUnion<ast_type_traits::DynTypedNode *,
> > +                                            ParentVector *>> ParentMap;
> >
> >   /// \brief Returns the parents of the given node.
> >   ///
> > @@ -2293,6 +2295,7 @@ private:
> >   friend class DeclContext;
> >   friend class DeclarationNameTable;
> >   void ReleaseDeclContextMaps();
> > +  void ReleaseParentMapEntries();
> >
> >   std::unique_ptr<ParentMap> AllParents;
> >
> >
> > Modified: cfe/trunk/lib/AST/ASTContext.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=209297&r1=209296&r2=209297&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/AST/ASTContext.cpp (original)
> > +++ cfe/trunk/lib/AST/ASTContext.cpp Wed May 21 08:28:59 2014
> > @@ -755,6 +755,8 @@ ASTContext::ASTContext(LangOptions& LOpt
> > }
> >
> > ASTContext::~ASTContext() {
> > +  ReleaseParentMapEntries();
> > +
> >   // Release the DenseMaps associated with DeclContext objects.
> >   // FIXME: Is this the ideal solution?
> >   ReleaseDeclContextMaps();
> > @@ -789,6 +791,18 @@ ASTContext::~ASTContext() {
> >   llvm::DeleteContainerSeconds(MangleNumberingContexts);
> > }
> >
> > +void ASTContext::ReleaseParentMapEntries() {
> > +  if (!AllParents) return;
> > +  for (const auto &Entry : *AllParents) {
> > +    if (Entry.second.is<ast_type_traits::DynTypedNode *>()) {
> > +      delete Entry.second.get<ast_type_traits::DynTypedNode *>();
> > +    } else {
> > +      assert(Entry.second.is<ParentVector *>());
> > +      delete Entry.second.get<ParentVector *>();
> > +    }
> > +  }
> > +}
> > +
> > void ASTContext::AddDeallocation(void (*Callback)(void*), void *Data) {
> >   Deallocations[Callback].push_back(Data);
> > }
> > @@ -8162,7 +8176,7 @@ namespace {
> >     bool TraverseNode(T *Node, bool(VisitorBase:: *traverse) (T *)) {
> >       if (!Node)
> >         return true;
> > -      if (ParentStack.size() > 0)
> > +      if (ParentStack.size() > 0) {
> >         // FIXME: Currently we add the same parent multiple times, for
> example
> >         // when we visit all subexpressions of template instantiations;
> this is
> >         // suboptimal, bug benign: the only way to visit those is with
> > @@ -8171,7 +8185,23 @@ namespace {
> >         // map. The main problem there is to implement hash functions /
> >         // comparison operators for all types that DynTypedNode supports
> that
> >         // do not have pointer identity.
> > -        (*Parents)[Node].push_back(ParentStack.back());
> > +        auto &NodeOrVector = (*Parents)[Node];
> > +        if (NodeOrVector.isNull()) {
> > +          NodeOrVector = new
> ast_type_traits::DynTypedNode(ParentStack.back());
> > +        } else if (NodeOrVector
> > +                       .template is<ast_type_traits::DynTypedNode *>())
> {
> > +          auto *Node =
> > +              NodeOrVector.template get<ast_type_traits::DynTypedNode
> *>();
> > +          auto *Vector = new ASTContext::ParentVector(1, *Node);
> > +          Vector->push_back(ParentStack.back());
> > +          NodeOrVector = Vector;
> > +          delete Node;
> > +        } else {
> > +          assert(NodeOrVector.template is<ASTContext::ParentVector
> *>());
> > +          NodeOrVector.template get<ASTContext::ParentVector
> *>()->push_back(
> > +              ParentStack.back());
> > +        }
> > +      }
> >
> ParentStack.push_back(ast_type_traits::DynTypedNode::create(*Node));
> >       bool Result = (this ->* traverse) (Node);
> >       ParentStack.pop_back();
> > @@ -8209,7 +8239,11 @@ ASTContext::getParents(const ast_type_tr
> >   if (I == AllParents->end()) {
> >     return ParentVector();
> >   }
> > -  return I->second;
> > +  if (I->second.is<ast_type_traits::DynTypedNode *>()) {
> > +    return ParentVector(1, *I->second.get<ast_type_traits::DynTypedNode
> *>());
> > +  }
> > +  const auto &Parents = *I->second.get<ParentVector *>();
> > +  return ParentVector(Parents.begin(), Parents.end());
> > }
> >
> > bool
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140521/628aacdd/attachment.html>