[PATCH] D69933: [ASTImporter] Limit imports of structs
jarin created this revision.
jarin added a reviewer: martong.
jarin added projects: LLDB, clang.
Herald added subscribers: cfe-commits, teemperor, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
jarin edited the summary of this revision.
This is a work in progress patch for discussion. The goal is to improve
performance of LLDB's expression evaluator.
During my investigations, I noticed that the AST importer is very eager to
import classes/structs that were already completed on the 'From' side, 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 a 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 the AST importer and test performance on several
expressions in an Unreal engine sample - preliminary results show this cuts
down evaluation time by roughly 50%.
This is work in progress, couple of lldb tests are failing (but hopefully
fixable). What would the experts here think? Is this a plausible direction?
—— 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']
...
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D69933
Files:
clang/lib/AST/ASTImporter.cpp
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2801,7 +2801,7 @@
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);
@@ -3440,6 +3440,9 @@
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;
}
@@ -5323,7 +5326,7 @@
D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
- if (D->isCompleteDefinition())
+ if (!Importer.isMinimalImport() && D->isCompleteDefinition())
if (Error Err = ImportDefinition(D, D2))
return std::move(Err);
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2801,7 +2801,7 @@
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);
@@ -3440,6 +3440,9 @@
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;
}
@@ -5323,7 +5326,7 @@
D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
-
[PATCH] D69933: [ASTImporter] Limit imports of structs
jarin updated this revision to Diff 239235.
jarin added a comment.
Herald added a subscriber: lldb-commits.
I changed the diff so that it does not touch Clang's AST importer, instead it
patches LLDB's wrapper of the AST importer.
The idea is to only import complete a record if the current evaluation asked
for them to be completed. In particular, if the current evaluation has not ask
for a record R to be completed and LLDB is being asked to import R, we supply
an incomplete version of R even if we happen to have a complete definition of R
lying around in parsed debug info.
This is achieved in a hacky way - if we have a complete record R, we pretend it
is incomplete by temporarily clearing R's isCompleteDefinition bit.
Interestingly, this hack is already used in
ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69933/new/
https://reviews.llvm.org/D69933
Files:
lldb/include/lldb/Symbol/ClangASTImporter.h
lldb/source/Symbol/ClangASTImporter.cpp
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -42,9 +42,12 @@
if (!delegate_sp)
return CompilerType();
+ delegate_sp->SetCompleted(src_qual_type->getAsTagDecl());
+
ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, &dst_clang_ast);
llvm::Expected ret_or_error = delegate_sp->Import(src_qual_type);
+
if (!ret_or_error) {
Log *log =
lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
@@ -252,6 +255,7 @@
if (auto *tag_decl = dyn_cast(decl)) {
if (auto *original_tag_decl = dyn_cast(original_decl)) {
if (original_tag_decl->isCompleteDefinition()) {
+m_delegate->SetCompleted(original_tag_decl);
m_delegate->ImportDefinitionTo(tag_decl, original_tag_decl);
tag_decl->setCompleteDefinition(true);
}
@@ -584,6 +588,8 @@
ImporterDelegateSP delegate_sp(
GetDelegate(&decl->getASTContext(), decl_origin.ctx));
+ delegate_sp->SetCompleted(decl_origin.decl);
+
ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp,
&decl->getASTContext());
if (delegate_sp)
@@ -855,6 +861,10 @@
ClangASTImporter::MapCompleter::~MapCompleter() { return; }
+void ClangASTImporter::ASTImporterDelegate::SetCompleted(Decl *from) {
+ m_completed_decls.insert(from);
+}
+
llvm::Expected
ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
if (m_std_handler) {
@@ -903,7 +913,20 @@
}
}
- return ASTImporter::ImportImpl(From);
+ CXXRecordDecl *record_decl_to_set_complete = nullptr;
+ if (CXXRecordDecl *record_decl = dyn_cast(From)) {
+if (record_decl->isCompleteDefinition() &&
+!record_decl->isAnonymousStructOrUnion() &&
+m_completed_decls.find(From) == m_completed_decls.end()) {
+ record_decl->setCompleteDefinition(false);
+ record_decl_to_set_complete = record_decl;
+}
+ }
+ llvm::Expected result = ASTImporter::ImportImpl(From);
+ if (record_decl_to_set_complete) {
+record_decl_to_set_complete->setCompleteDefinition(true);
+ }
+ return result;
}
void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -231,6 +231,8 @@
clang::Decl *GetOriginalDecl(clang::Decl *To) override;
+void SetCompleted(clang::Decl *from);
+
void SetImportListener(NewDeclListener *listener) {
assert(m_new_decl_listener == nullptr && "Already attached a listener?");
m_new_decl_listener = listener;
@@ -246,6 +248,7 @@
/// were created from the 'std' C++ module to prevent that the Importer
/// tries to sync them with the broken equivalent in the debug info AST.
llvm::SmallPtrSet m_decls_to_ignore;
+llvm::SmallPtrSet m_completed_decls;
ClangASTImporter &m_master;
clang::ASTContext *m_source_ctx;
CxxModuleHandler *m_std_handler = nullptr;
@@ -257,6 +260,7 @@
typedef llvm::DenseMap DelegateMap;
typedef llvm::DenseMap
NamespaceMetaMap;
+ typedef std::set CompletedRecordSet;
struct ASTContextMetadata {
ASTContextMetadata(clang::ASTContext *dst_ctx)
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -42,9 +42,12 @@
if (!delegate_sp)
return CompilerType();
+ delegate_sp->SetCompleted(src_qual_type->getAsTagDecl());
+
ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, &dst_clang_ast);
llvm::Expected ret_or_er
[PATCH] D73166: [ASTImporter] Properly delete decls from SavedImportPaths
jarin created this revision. jarin added reviewers: martong, teemperor. Herald added subscribers: cfe-commits, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a reviewer: shafik. Herald added a project: clang. We see a significant regression (~40% slower on large codebases) in expression evaluation after https://reviews.llvm.org/rL364771. A sampling profile shows the extra time is spent in SavedImportPathsTy::operator[] when called from ASTImporter::Import. I believe this is because ASTImporter::Import adds an element to the SavedImportPaths map for each decl unconditionally (see https://github.com/llvm/llvm-project/blob/7b81c3f8793d30a4285095a9b67dcfca2117916c/clang/lib/AST/ASTImporter.cpp#L8256). To fix this, we call SavedImportPathsTy::erase on the declaration rather than clearing its value vector. That way we do not accidentally introduce new empty elements. (With this patch the performance is restored, and we do not see SavedImportPathsTy::operator[] in the profile anymore.) Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D73166 Files: clang/lib/AST/ASTImporter.cpp Index: clang/lib/AST/ASTImporter.cpp === --- clang/lib/AST/ASTImporter.cpp +++ clang/lib/AST/ASTImporter.cpp @@ -8241,7 +8241,7 @@ // FIXME Should we remove these Decls from the LookupTable, // and from ImportedFromDecls? } -SavedImportPaths[FromD].clear(); +SavedImportPaths.erase(FromD); // Do not return ToDOrErr, error was taken out of it. return make_error(ErrOut); @@ -8274,7 +8274,7 @@ Imported(FromD, ToD); updateFlags(FromD, ToD); - SavedImportPaths[FromD].clear(); + SavedImportPaths.erase(FromD); return ToDOrErr; } Index: clang/lib/AST/ASTImporter.cpp === --- clang/lib/AST/ASTImporter.cpp +++ clang/lib/AST/ASTImporter.cpp @@ -8241,7 +8241,7 @@ // FIXME Should we remove these Decls from the LookupTable, // and from ImportedFromDecls? } -SavedImportPaths[FromD].clear(); +SavedImportPaths.erase(FromD); // Do not return ToDOrErr, error was taken out of it. return make_error(ErrOut); @@ -8274,7 +8274,7 @@ Imported(FromD, ToD); updateFlags(FromD, ToD); - SavedImportPaths[FromD].clear(); + SavedImportPaths.erase(FromD); return ToDOrErr; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73166: [ASTImporter] Properly delete decls from SavedImportPaths
jarin added a comment. Thanks for the review! I am not a committer; could you land the fix for me? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73166/new/ https://reviews.llvm.org/D73166 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
