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

[Bruno Ricci via cfe-dev]

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.

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 ?)

Bruno