[lldb-dev] Slow expression evaluation (ASTImporter is too eager?)

Thu Nov 7 09:14:08 PST 2019

Hi Jaroslav,

Thanks for working on this. Still, there are things that are unclear for me.
I see that `LayoutRecordType` is called superfluously during the second
evaluation of `c2.x`, ok.

But, could you explain why LLDB is calling that multiple times? Maybe it
thinks a type is not completed while it is? As far as I know we keep track
which RecordDecl needs completeion in LLDB. At least, we do not store that
info in clang::ASTImporter. Minimal import gives a CXXRecordDecl whose
`DefinitionData` is set, even though the members are not imported the
definition data is there! So, there is no way for clang::ASTImporter to
know which RecordDecl had been imported in a minimal way.

I suspect there are multiple invocations of ASTImporter::ImportDefinition
with C2, C1, C0 within ClangASTSource (could you please check that?). And
`ImportDefinition` will blindly import the full definitions recursively
even if the minimal import is set (see ASTNodeImporter::IDK_Everything).
The patch would change this behavior which I'd like to avoid if possible. I
think first we should discover why there are multiple invocations of
ASTImporter::ImportDefinition from within LLDB.

Gabor

On Wed, Nov 6, 2019 at 11:21 PM Raphael “Teemperor” Isemann via lldb-dev <
lldb-dev at lists.llvm.org> wrote:

> Can you post that patch on Phabricator with an '[ASTImporter]’ as a name
> prefix? This way the ASTImporter folks will see your patch (The ASTImporter
> is more of a Clang thing, so they might not read lldb-dev). Also it makes
> it easier to see/test your patch :)
>
> (And +Gabor just in case)
>
> > On Nov 6, 2019, at 10:25 PM, Jaroslav Sevcik via lldb-dev <
> lldb-dev at lists.llvm.org> wrote:
> >
> > Hello,
> >
> > I noticed that the AST importer is very eager to import classes/structs
> that were already completed, even if they are not needed. This is best
> illustrated with an example.
> >
> > struct C0 { int x = 0; };
> > struct C1 { int x = 1; C0* c0 = 0; };
> > struct C2 { int x = 2; C1* c1 = 0; };
> >
> > int main() {
> >   C0 c0;
> >   C1 c1;
> >   C2 c2;
> >
> >   return 0;  // break here
> > }
> >
> > When we evaluate “c2.x” in LLDB, AST importer completes and imports only
> class C2. This is working as intended. Similarly, evaluating “c1.x” imports
> just C1 and “c0.x” imports C0. However, if we evaluate “c2.x” after
> evaluating “c1.x” and “c0.x”, the importer suddenly imports both C1 and C0
> (in addition to C2). See a log from a lldb session at the end of this email
> for illustration.
> >
> > I believe the culprit here is the following code at the end of the
> ASTNodeImporter::VisitRecordDecl method:
> >
> >   if (D->isCompleteDefinition())
> >     if (Error Err = ImportDefinition(D, D2, IDK_Default))
> >       return std::move(Err);
> >
> > This will import a definition of class from LLDB if LLDB already happens
> to have a complete definition from before. For large programs, this can
> lead to importing very large chunks of ASTs even if they are not needed. I
> have tried to remove the code above from clang and test performance on
> several expressions in an Unreal engine sample - preliminary results show
> this could cut down evaluation time by roughly 50%.
> >
> > My prototype patch is below; note that couple of lldb tests are failing
> with a wrong error message, this is a work in progress. What would the
> experts here think? Is this a plausible direction?
> >
> > Cheers, Jaro
> >
> > diff --git a/clang/lib/AST/ASTImporter.cpp
> b/clang/lib/AST/ASTImporter.cpp
> > index 54acca7dc62..ebbce5c66c7 100644
> > --- a/clang/lib/AST/ASTImporter.cpp
> > +++ b/clang/lib/AST/ASTImporter.cpp
> > @@ -2799,7 +2799,7 @@ ExpectedDecl
> ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
> >    if (D->isAnonymousStructOrUnion())
> >      D2->setAnonymousStructOrUnion(true);
> >
> > -  if (D->isCompleteDefinition())
> > +  if (!Importer.isMinimalImport() && D->isCompleteDefinition())
> >      if (Error Err = ImportDefinition(D, D2, IDK_Default))
> >        return std::move(Err);
> >
> > @@ -3438,6 +3438,9 @@ ExpectedDecl
> ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {
> >    if (ToInitializer)
> >      ToField->setInClassInitializer(ToInitializer);
> >    ToField->setImplicit(D->isImplicit());
> > +  if (CXXRecordDecl *FieldType = D->getType()->getAsCXXRecordDecl())
> > +       if (Error Err = ImportDefinitionIfNeeded(FieldType))
> > +         return std::move(Err);
> >    LexicalDC->addDeclInternal(ToField);
> >    return ToField;
> >  }
> > @@ -5307,7 +5310,7 @@ ExpectedDecl
> ASTNodeImporter::VisitClassTemplateSpecializationDecl(
> >
> >    D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
> >
> > -  if (D->isCompleteDefinition())
> > +  if (!Importer.isMinimalImport() && D->isCompleteDefinition())
> >      if (Error Err = ImportDefinition(D, D2))
> >        return std::move(Err);
> >
> >
> >
> > —— lldb session illustrating the unnecessary imports —-
> > This shows that evaluation of “c2.x” after evaluation “c1.x” and “c0.x”
> calls to LayoutRecordType for C2, C1 and C0.
> > $ lldb a.out
> > (lldb) b h.cc:10
> > Breakpoint 1: where = a.out`main + 44 at h.cc:10:3, address = ...
> > (lldb) r
> > ... Process stopped ...
> > (lldb) log enable lldb expr
> > (lldb) p c2.x
> > ...
> > LayoutRecordType[6] ... for (RecordDecl*)0x... [name = 'C2']
> > ...
> > (lldb) p c1.x
> > ...
> > LayoutRecordType[7] ... for (RecordDecl*)0x... [name = 'C1']
> > ...
> > (lldb) p c0.x
> > ...
> > LayoutRecordType[8] ... for (RecordDecl*)0x... [name = 'C0']
> > ...
> > (lldb) p c2.x
> > ...
> > LayoutRecordType[9] ... for (RecordDecl*)0x... [name = 'C2']
> > LayoutRecordType[10] ... for (RecordDecl*)0x... [name = 'C1']
> > LayoutRecordType[11] ... for (RecordDecl*)0x... [name = 'C0']
> > ...
> >
> > _______________________________________________
> > lldb-dev mailing list
> > lldb-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
> _______________________________________________
> lldb-dev mailing list
> lldb-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20191107/6adc33e2/attachment.html>