Remove static CladUtils AST caches - #1856
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
vgvassilev
left a comment
There was a problem hiding this comment.
That patch essentially makes things slower. While it is right in its core that we can have multiple instances of clad in the same process it is not yet a practical use-case. An alternative approach would be to provide some per plugin instance cache area which is somehow shared with the rest of the infrastructure. I think overall it is not worth the effort and the complexity right now.
bf419fc to
26b4acf
Compare
| } | ||
|
|
||
| namespace { | ||
| NamespaceDecl* CachedCladNS = nullptr; |
There was a problem hiding this comment.
warning: variable 'CachedCladNS' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
NamespaceDecl* CachedCladNS = nullptr;
^| } | ||
|
|
||
| namespace { | ||
| NamespaceDecl* CachedCladNS = nullptr; |
There was a problem hiding this comment.
warning: variable 'CachedCladNS' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
NamespaceDecl* CachedCladNS = nullptr;
^|
|
||
| namespace { | ||
| NamespaceDecl* CachedCladNS = nullptr; | ||
| QualType CachedRestoreTrackerTy; |
There was a problem hiding this comment.
warning: variable 'CachedRestoreTrackerTy' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
QualType CachedRestoreTrackerTy;
^| namespace { | ||
| NamespaceDecl* CachedCladNS = nullptr; | ||
| QualType CachedRestoreTrackerTy; | ||
| TemplateDecl* CachedMatrixDecl = nullptr; |
There was a problem hiding this comment.
warning: variable 'CachedMatrixDecl' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
TemplateDecl* CachedMatrixDecl = nullptr;
^| namespace { | ||
| NamespaceDecl* CachedCladNS = nullptr; | ||
| QualType CachedRestoreTrackerTy; | ||
| TemplateDecl* CachedMatrixDecl = nullptr; |
There was a problem hiding this comment.
warning: variable 'CachedMatrixDecl' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
TemplateDecl* CachedMatrixDecl = nullptr;
^| NamespaceDecl* CachedCladNS = nullptr; | ||
| QualType CachedRestoreTrackerTy; | ||
| TemplateDecl* CachedMatrixDecl = nullptr; | ||
| TemplateDecl* CachedArrayDecl = nullptr; |
There was a problem hiding this comment.
warning: variable 'CachedArrayDecl' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
TemplateDecl* CachedArrayDecl = nullptr;
^| NamespaceDecl* CachedCladNS = nullptr; | ||
| QualType CachedRestoreTrackerTy; | ||
| TemplateDecl* CachedMatrixDecl = nullptr; | ||
| TemplateDecl* CachedArrayDecl = nullptr; |
There was a problem hiding this comment.
warning: variable 'CachedArrayDecl' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
TemplateDecl* CachedArrayDecl = nullptr;
^| QualType CachedRestoreTrackerTy; | ||
| TemplateDecl* CachedMatrixDecl = nullptr; | ||
| TemplateDecl* CachedArrayDecl = nullptr; | ||
| TemplateDecl* CachedArrayRefDecl = nullptr; |
There was a problem hiding this comment.
warning: variable 'CachedArrayRefDecl' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
TemplateDecl* CachedArrayRefDecl = nullptr;
^| QualType CachedRestoreTrackerTy; | ||
| TemplateDecl* CachedMatrixDecl = nullptr; | ||
| TemplateDecl* CachedArrayDecl = nullptr; | ||
| TemplateDecl* CachedArrayRefDecl = nullptr; |
There was a problem hiding this comment.
warning: variable 'CachedArrayRefDecl' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
TemplateDecl* CachedArrayRefDecl = nullptr;
^| TemplateDecl* CachedMatrixDecl = nullptr; | ||
| TemplateDecl* CachedArrayDecl = nullptr; | ||
| TemplateDecl* CachedArrayRefDecl = nullptr; | ||
| TemplateDecl* CachedTagDecl = nullptr; |
There was a problem hiding this comment.
warning: variable 'CachedTagDecl' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
TemplateDecl* CachedTagDecl = nullptr;
^26b4acf to
8be8e96
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@vgvassilev thanks again for the review. The first version was trying to avoid reusing AST-owned cached declarations/types across ASTContexts. After testing and looking at the hot path more carefully, I see why removing the caches entirely was the wrong tradeoff: it avoided stale pointers by making normal differentiation redo namespace/type/template lookups. I reworked the PR to keep the cache fast path and reset the cached AST-owned values from Validation:
I think this version addresses the practical stale-cache concern while preserving the existing performance model. If this still feels too narrow or not worth carrying right now, I am fine closing the PR. |
|
clang-tidy review says "All clean, LGTM! 👍" |
Problem
Several
CladUtils.cpphelpers cache Clang AST-owned declarations or types in function-localstaticvariables:GetCladNamespace()cachedNamespaceDecl*GetRestoreTrackerType()cachedQualTypeGetCladMatrixOfType(),GetCladArrayOfType(),GetCladArrayRefOfType(), andGetCladTagOfType()cachedTemplateDecl*Those objects are tied to the current
Sema/ASTContext. Reusing them across translation units in a long-lived process can leave stale AST pointers, similar to theLookupResultcache issue fixed in #1838.Solution
Remove those function-local static caches and perform the lookup for the current
Semawhen needed.Testing
git diff --check HEADLocal build was not run because this Windows environment does not have
clang,llvm-config,cmake, orclang-formaton PATH.