[cfe-dev] Avoiding unnecessary calls to Decl::getASTContext

[Richard Smith via cfe-dev]

On Sun, 16 Dec 2018, 14:27 Bruno Ricci via cfe-dev <cfe-dev at lists.llvm.org
wrote:

> On 16/12/2018 18:55, Richard Smith wrote:
> > On Sat, 15 Dec 2018, 09:57 Bruno Ricci via cfe-dev <
> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org> wrote:
> >
> >     I have added the profile data to the first patch (
> https://reviews.llvm.org/D55658)
> >
> >     The top 20 callers of getASTContext and getParentASTContext are:
> >     (with the number of calls to getTranslationUnitDecl, edited for
> readability)
> >
> >     5399586 : DeclContext::lookup
> >     801684 : VarDecl::getMemberSpecializationInfo
> >     551928 : FunctionDecl::getMinRequiredArguments
> >     401464 : CXXRecordDecl::lookupInBases
> >     338614 : Decl::getAttrs
> >     311651 : DeclContext::makeDeclVisibleInContextImpl
> >     173684 : Decl::getASTMutationListener
> >     164264 : CXXConstructorDecl::isSpecializationCopyingObject
> >     154066 : CXXMethodDecl::size_overridden_methods
> >     141622 : CXXRecordDecl::getVisibleConversionFunctions
> >     120440 : ClassTemplateSpecializationDecl::Profile
> >     116303 : VarDecl::isThisDeclarationADefinition
> >     106208 : CXXRecordDecl::conversion_begin
> >     106208 : CXXRecordDecl::conversion_end
> >     99075 : FunctionDecl::setParams
> >     95910 : RedeclarableTemplateDecl::findSpecializationImpl
> >     57043 : CXXRecordDecl::getDestructor
> >     56668 : VarDecl::getDefinition
> >     52140 : TagDecl::startDefinition
> >     48266 : CXXConstructorDecl::isCopyOrMoveConstructor
> >     37407 : FunctionDecl::getBody
> >
> >
> > Given your data that perhaps 1.5-2% of the time building boost is
> finding the AST context, it definitely seems worthwhile to avoid doing so
> for lookup. The next three seem reasonable to address too, if you feel so
> inclined, but I'd not go any further than that -- the long tail seems
> negligible in comparison.
>
> Just to clarify, the 1.5%-2% figure is an estimation for the time spent
> obtaining the
> AST context when parsing all of Boost without doing any code generation
> (ie: -fsyntax-only).
>
> I generally agree about not bothering too much with the long tail, with
> perhaps two exceptions:
>
> 1.) Some of these are really easy to fix. For example
> FunctionDecl::setParams is almost only
>     called in Sema and so the context is already easily available.
>

Sure, non-disruptive cleanups for this seem good where possible. (But for
example, getAttrs is called from a large number of places and it'd be
really awkward to require an ASTContext, so I'd leave that one.)

2.) The numbers shown above are the number of calls to
> getTranslationUnitDecl. A better metric
>     would be the number of iterations needed to reach the translation unit
> declaration. With
>     that in mind it would also make sense to pass the AST context to some
> of the above where
>     the declaration is likely to occur nested deep in the declaration
> context chain
>     (VarDecls for example).
>
> If there are no objections I will submit some patches next week to do this
> change, hopefully
> in small increments. In any case this will require small (but
> straightforward) changes to at
> least clang-tools-extra and lldb. (Am I forgetting any project here ?)
>

I think it's just those two. Thank you!

Bruno
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20181217/14d48834/attachment.html>