<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 21, 2014 at 6:10 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
</blockquote><div><br></div><div>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.</div><div>If we change the ParentVector to a SmallVector<T, 2> we'll also use that for getParents() which mostly returns a single element.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
Jordan<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On May 21, 2014, at 6:28 , Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
<br>
> Author: klimek<br>
> Date: Wed May 21 08:28:59 2014<br>
> New Revision: 209297<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=209297&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=209297&view=rev</a><br>
> Log:<br>
> Make the parent-map use significantly less memory.<br>
><br>
> On test files I ran this on, memory consumption overall went down from<br>
> 2.5G to 2G, without performance regressions.<br>
> I also investigated making DynTypedNode by itself smaller (by pulling<br>
> out pointers for everything that doesn't fit in 8 bytes). This led to<br>
> another 200-300MB saved, but also introduced a significant regression in<br>
> performance due to the memory management overhead.<br>
><br>
> Modified:<br>
>    cfe/trunk/include/clang/AST/ASTContext.h<br>
>    cfe/trunk/lib/AST/ASTContext.cpp<br>
><br>
> Modified: cfe/trunk/include/clang/AST/ASTContext.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=209297&r1=209296&r2=209297&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=209297&r1=209296&r2=209297&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/include/clang/AST/ASTContext.h (original)<br>
> +++ cfe/trunk/include/clang/AST/ASTContext.h Wed May 21 08:28:59 2014<br>
> @@ -424,7 +424,9 @@ public:<br>
>   typedef llvm::SmallVector<ast_type_traits::DynTypedNode, 1> ParentVector;<br>
><br>
>   /// \brief Maps from a node to its parents.<br>
> -  typedef llvm::DenseMap<const void *, ParentVector> ParentMap;<br>
> +  typedef llvm::DenseMap<const void *,<br>
> +                         llvm::PointerUnion<ast_type_traits::DynTypedNode *,<br>
> +                                            ParentVector *>> ParentMap;<br>
><br>
>   /// \brief Returns the parents of the given node.<br>
>   ///<br>
> @@ -2293,6 +2295,7 @@ private:<br>
>   friend class DeclContext;<br>
>   friend class DeclarationNameTable;<br>
>   void ReleaseDeclContextMaps();<br>
> +  void ReleaseParentMapEntries();<br>
><br>
>   std::unique_ptr<ParentMap> AllParents;<br>
><br>
><br>
> Modified: cfe/trunk/lib/AST/ASTContext.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=209297&r1=209296&r2=209297&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=209297&r1=209296&r2=209297&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/lib/AST/ASTContext.cpp (original)<br>
> +++ cfe/trunk/lib/AST/ASTContext.cpp Wed May 21 08:28:59 2014<br>
> @@ -755,6 +755,8 @@ ASTContext::ASTContext(LangOptions& LOpt<br>
> }<br>
><br>
> ASTContext::~ASTContext() {<br>
> +  ReleaseParentMapEntries();<br>
> +<br>
>   // Release the DenseMaps associated with DeclContext objects.<br>
>   // FIXME: Is this the ideal solution?<br>
>   ReleaseDeclContextMaps();<br>
> @@ -789,6 +791,18 @@ ASTContext::~ASTContext() {<br>
>   llvm::DeleteContainerSeconds(MangleNumberingContexts);<br>
> }<br>
><br>
> +void ASTContext::ReleaseParentMapEntries() {<br>
> +  if (!AllParents) return;<br>
> +  for (const auto &Entry : *AllParents) {<br>
> +    if (<a href="http://Entry.second.is" target="_blank">Entry.second.is</a><ast_type_traits::DynTypedNode *>()) {<br>
> +      delete Entry.second.get<ast_type_traits::DynTypedNode *>();<br>
> +    } else {<br>
> +      assert(<a href="http://Entry.second.is" target="_blank">Entry.second.is</a><ParentVector *>());<br>
> +      delete Entry.second.get<ParentVector *>();<br>
> +    }<br>
> +  }<br>
> +}<br>
> +<br>
> void ASTContext::AddDeallocation(void (*Callback)(void*), void *Data) {<br>
>   Deallocations[Callback].push_back(Data);<br>
> }<br>
> @@ -8162,7 +8176,7 @@ namespace {<br>
>     bool TraverseNode(T *Node, bool(VisitorBase:: *traverse) (T *)) {<br>
>       if (!Node)<br>
>         return true;<br>
> -      if (ParentStack.size() > 0)<br>
> +      if (ParentStack.size() > 0) {<br>
>         // FIXME: Currently we add the same parent multiple times, for example<br>
>         // when we visit all subexpressions of template instantiations; this is<br>
>         // suboptimal, bug benign: the only way to visit those is with<br>
> @@ -8171,7 +8185,23 @@ namespace {<br>
>         // map. The main problem there is to implement hash functions /<br>
>         // comparison operators for all types that DynTypedNode supports that<br>
>         // do not have pointer identity.<br>
> -        (*Parents)[Node].push_back(ParentStack.back());<br>
> +        auto &NodeOrVector = (*Parents)[Node];<br>
> +        if (NodeOrVector.isNull()) {<br>
> +          NodeOrVector = new ast_type_traits::DynTypedNode(ParentStack.back());<br>
> +        } else if (NodeOrVector<br>
> +                       .template is<ast_type_traits::DynTypedNode *>()) {<br>
> +          auto *Node =<br>
> +              NodeOrVector.template get<ast_type_traits::DynTypedNode *>();<br>
> +          auto *Vector = new ASTContext::ParentVector(1, *Node);<br>
> +          Vector->push_back(ParentStack.back());<br>
> +          NodeOrVector = Vector;<br>
> +          delete Node;<br>
> +        } else {<br>
> +          assert(NodeOrVector.template is<ASTContext::ParentVector *>());<br>
> +          NodeOrVector.template get<ASTContext::ParentVector *>()->push_back(<br>
> +              ParentStack.back());<br>
> +        }<br>
> +      }<br>
>       ParentStack.push_back(ast_type_traits::DynTypedNode::create(*Node));<br>
>       bool Result = (this ->* traverse) (Node);<br>
>       ParentStack.pop_back();<br>
> @@ -8209,7 +8239,11 @@ ASTContext::getParents(const ast_type_tr<br>
>   if (I == AllParents->end()) {<br>
>     return ParentVector();<br>
>   }<br>
> -  return I->second;<br>
> +  if (I-><a href="http://second.is" target="_blank">second.is</a><ast_type_traits::DynTypedNode *>()) {<br>
> +    return ParentVector(1, *I->second.get<ast_type_traits::DynTypedNode *>());<br>
> +  }<br>
> +  const auto &Parents = *I->second.get<ParentVector *>();<br>
> +  return ParentVector(Parents.begin(), Parents.end());<br>
> }<br>
><br>
> bool<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</div></div></blockquote></div><br></div></div>