<div dir="ltr">Hi Jaroslav,<div><br></div><div>Thanks for working on this. Still, there are things that are unclear for me.</div><div>I see that `LayoutRecordType` is called superfluously during the second evaluation of `c2.x`, ok.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Gabor</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 6, 2019 at 11:21 PM Raphael “Teemperor” Isemann via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org">lldb-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 :)<br>
<br>
(And +Gabor just in case)<br>
<br>
> On Nov 6, 2019, at 10:25 PM, Jaroslav Sevcik via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>> wrote:<br>
> <br>
> Hello,<br>
> <br>
> 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. <br>
> <br>
> struct C0 { int x = 0; };<br>
> struct C1 { int x = 1; C0* c0 = 0; };<br>
> struct C2 { int x = 2; C1* c1 = 0; };<br>
> <br>
> int main() {<br>
> C0 c0;<br>
> C1 c1;<br>
> C2 c2;<br>
> <br>
> return 0; // break here<br>
> }<br>
> <br>
> 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.<br>
> <br>
> I believe the culprit here is the following code at the end of the ASTNodeImporter::VisitRecordDecl method:<br>
> <br>
> if (D->isCompleteDefinition())<br>
> if (Error Err = ImportDefinition(D, D2, IDK_Default))<br>
> return std::move(Err);<br>
> <br>
> 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%. <br>
> <br>
> 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?<br>
> <br>
> Cheers, Jaro<br>
> <br>
> diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp<br>
> index 54acca7dc62..ebbce5c66c7 100644<br>
> --- a/clang/lib/AST/ASTImporter.cpp<br>
> +++ b/clang/lib/AST/ASTImporter.cpp<br>
> @@ -2799,7 +2799,7 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {<br>
> if (D->isAnonymousStructOrUnion())<br>
> D2->setAnonymousStructOrUnion(true);<br>
> <br>
> - if (D->isCompleteDefinition())<br>
> + if (!Importer.isMinimalImport() && D->isCompleteDefinition())<br>
> if (Error Err = ImportDefinition(D, D2, IDK_Default))<br>
> return std::move(Err);<br>
> <br>
> @@ -3438,6 +3438,9 @@ ExpectedDecl ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {<br>
> if (ToInitializer)<br>
> ToField->setInClassInitializer(ToInitializer);<br>
> ToField->setImplicit(D->isImplicit());<br>
> + if (CXXRecordDecl *FieldType = D->getType()->getAsCXXRecordDecl())<br>
> + if (Error Err = ImportDefinitionIfNeeded(FieldType))<br>
> + return std::move(Err);<br>
> LexicalDC->addDeclInternal(ToField);<br>
> return ToField;<br>
> }<br>
> @@ -5307,7 +5310,7 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateSpecializationDecl(<br>
> <br>
> D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());<br>
> <br>
> - if (D->isCompleteDefinition())<br>
> + if (!Importer.isMinimalImport() && D->isCompleteDefinition())<br>
> if (Error Err = ImportDefinition(D, D2))<br>
> return std::move(Err); <br>
> <br>
> <br>
> <br>
> —— lldb session illustrating the unnecessary imports —-<br>
> This shows that evaluation of “c2.x” after evaluation “c1.x” and “c0.x” calls to LayoutRecordType for C2, C1 and C0.<br>
> $ lldb a.out<br>
> (lldb) b h.cc:10<br>
> Breakpoint 1: where = a.out`main + 44 at h.cc:10:3, address = ...<br>
> (lldb) r<br>
> ... Process stopped ...<br>
> (lldb) log enable lldb expr<br>
> (lldb) p c2.x<br>
> ...<br>
> LayoutRecordType[6] ... for (RecordDecl*)0x... [name = 'C2']<br>
> ...<br>
> (lldb) p c1.x<br>
> ...<br>
> LayoutRecordType[7] ... for (RecordDecl*)0x... [name = 'C1']<br>
> ...<br>
> (lldb) p c0.x<br>
> ...<br>
> LayoutRecordType[8] ... for (RecordDecl*)0x... [name = 'C0']<br>
> ...<br>
> (lldb) p c2.x<br>
> ...<br>
> LayoutRecordType[9] ... for (RecordDecl*)0x... [name = 'C2']<br>
> LayoutRecordType[10] ... for (RecordDecl*)0x... [name = 'C1']<br>
> LayoutRecordType[11] ... for (RecordDecl*)0x... [name = 'C0']<br>
> ...<br>
> <br>
> _______________________________________________<br>
> lldb-dev mailing list<br>
> <a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
<br>
_______________________________________________<br>
lldb-dev mailing list<br>
<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
</blockquote></div>