[clang-tidy][NFC] optimize cache for config option #121406
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Current implement will cache ConfigOptions for each path, it will create lots of copy of options when project have deep nested folder structure. New implement use vector to store options and only cache the index to reduce memory usage and avoid meanless copy
|
@llvm/pr-subscribers-clang-tidy Author: Congcong Cai (HerrCai0907) ChangesCurrent implement will cache Full diff: https://github.com/llvm/llvm-project/pull/121406.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index 445c7f85c900c6..9ecb255d916e9b 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -337,33 +337,34 @@ FileOptionsBaseProvider::FileOptionsBaseProvider(
void FileOptionsBaseProvider::addRawFileOptions(
llvm::StringRef AbsolutePath, std::vector<OptionsSource> &CurOptions) {
auto CurSize = CurOptions.size();
-
// Look for a suitable configuration file in all parent directories of the
// file. Start with the immediate parent directory and move up.
- StringRef Path = llvm::sys::path::parent_path(AbsolutePath);
- for (StringRef CurrentPath = Path; !CurrentPath.empty();
- CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
- std::optional<OptionsSource> Result;
-
- auto Iter = CachedOptions.find(CurrentPath);
- if (Iter != CachedOptions.end())
- Result = Iter->second;
-
- if (!Result)
- Result = tryReadConfigFile(CurrentPath);
-
- if (Result) {
- // Store cached value for all intermediate directories.
- while (Path != CurrentPath) {
+ StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath);
+ auto MemorizedConfigFile =
+ [this, &RootPath](StringRef CurrentPath) -> std::optional<OptionsSource> {
+ auto Iter = CachedOptions.Memorized.find(CurrentPath);
+ if (Iter != CachedOptions.Memorized.end())
+ return CachedOptions.Storage[Iter->second];
+ std::optional<OptionsSource> OptionsSource = tryReadConfigFile(CurrentPath);
+ if (OptionsSource) {
+ const size_t Index = CachedOptions.Storage.size();
+ CachedOptions.Storage.emplace_back(OptionsSource.value());
+ while (RootPath != CurrentPath) {
LLVM_DEBUG(llvm::dbgs()
- << "Caching configuration for path " << Path << ".\n");
- if (!CachedOptions.count(Path))
- CachedOptions[Path] = *Result;
- Path = llvm::sys::path::parent_path(Path);
+ << "Caching configuration for path " << RootPath << ".\n");
+ CachedOptions.Memorized[RootPath] = Index;
+ RootPath = llvm::sys::path::parent_path(RootPath);
}
- CachedOptions[Path] = *Result;
-
- CurOptions.push_back(*Result);
+ CachedOptions.Memorized[CurrentPath] = Index;
+ RootPath = llvm::sys::path::parent_path(CurrentPath);
+ }
+ return OptionsSource;
+ };
+ for (StringRef CurrentPath = RootPath; !CurrentPath.empty();
+ CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
+ if (std::optional<OptionsSource> Result =
+ MemorizedConfigFile(CurrentPath)) {
+ CurOptions.emplace_back(Result.value());
if (!Result->first.InheritParentConfig.value_or(false))
break;
}
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.h b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
index 85d5a02ebbc1bc..568f60cf98b21f 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -241,7 +241,10 @@ class FileOptionsBaseProvider : public DefaultOptionsProvider {
/// \c ConfigHandlers.
std::optional<OptionsSource> tryReadConfigFile(llvm::StringRef Directory);
- llvm::StringMap<OptionsSource> CachedOptions;
+ struct OptionsCache {
+ llvm::StringMap<size_t> Memorized;
+ llvm::SmallVector<OptionsSource, 4U> Storage;
+ } CachedOptions;
ClangTidyOptions OverrideOptions;
ConfigFileHandlers ConfigHandlers;
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Congcong Cai (HerrCai0907) ChangesCurrent implement will cache Full diff: https://github.com/llvm/llvm-project/pull/121406.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index 445c7f85c900c6..9ecb255d916e9b 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -337,33 +337,34 @@ FileOptionsBaseProvider::FileOptionsBaseProvider(
void FileOptionsBaseProvider::addRawFileOptions(
llvm::StringRef AbsolutePath, std::vector<OptionsSource> &CurOptions) {
auto CurSize = CurOptions.size();
-
// Look for a suitable configuration file in all parent directories of the
// file. Start with the immediate parent directory and move up.
- StringRef Path = llvm::sys::path::parent_path(AbsolutePath);
- for (StringRef CurrentPath = Path; !CurrentPath.empty();
- CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
- std::optional<OptionsSource> Result;
-
- auto Iter = CachedOptions.find(CurrentPath);
- if (Iter != CachedOptions.end())
- Result = Iter->second;
-
- if (!Result)
- Result = tryReadConfigFile(CurrentPath);
-
- if (Result) {
- // Store cached value for all intermediate directories.
- while (Path != CurrentPath) {
+ StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath);
+ auto MemorizedConfigFile =
+ [this, &RootPath](StringRef CurrentPath) -> std::optional<OptionsSource> {
+ auto Iter = CachedOptions.Memorized.find(CurrentPath);
+ if (Iter != CachedOptions.Memorized.end())
+ return CachedOptions.Storage[Iter->second];
+ std::optional<OptionsSource> OptionsSource = tryReadConfigFile(CurrentPath);
+ if (OptionsSource) {
+ const size_t Index = CachedOptions.Storage.size();
+ CachedOptions.Storage.emplace_back(OptionsSource.value());
+ while (RootPath != CurrentPath) {
LLVM_DEBUG(llvm::dbgs()
- << "Caching configuration for path " << Path << ".\n");
- if (!CachedOptions.count(Path))
- CachedOptions[Path] = *Result;
- Path = llvm::sys::path::parent_path(Path);
+ << "Caching configuration for path " << RootPath << ".\n");
+ CachedOptions.Memorized[RootPath] = Index;
+ RootPath = llvm::sys::path::parent_path(RootPath);
}
- CachedOptions[Path] = *Result;
-
- CurOptions.push_back(*Result);
+ CachedOptions.Memorized[CurrentPath] = Index;
+ RootPath = llvm::sys::path::parent_path(CurrentPath);
+ }
+ return OptionsSource;
+ };
+ for (StringRef CurrentPath = RootPath; !CurrentPath.empty();
+ CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
+ if (std::optional<OptionsSource> Result =
+ MemorizedConfigFile(CurrentPath)) {
+ CurOptions.emplace_back(Result.value());
if (!Result->first.InheritParentConfig.value_or(false))
break;
}
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.h b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
index 85d5a02ebbc1bc..568f60cf98b21f 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -241,7 +241,10 @@ class FileOptionsBaseProvider : public DefaultOptionsProvider {
/// \c ConfigHandlers.
std::optional<OptionsSource> tryReadConfigFile(llvm::StringRef Directory);
- llvm::StringMap<OptionsSource> CachedOptions;
+ struct OptionsCache {
+ llvm::StringMap<size_t> Memorized;
+ llvm::SmallVector<OptionsSource, 4U> Storage;
+ } CachedOptions;
ClangTidyOptions OverrideOptions;
ConfigFileHandlers ConfigHandlers;
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
|
|
ping |
5chmidti
approved these changes
Jan 9, 2025
| StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath); | ||
| auto MemorizedConfigFile = | ||
| [this, &RootPath](StringRef CurrentPath) -> std::optional<OptionsSource> { | ||
| auto Iter = CachedOptions.Memorized.find(CurrentPath); |
There was a problem hiding this comment.
nit: can be const, and can be declared as the if-init var to reduce its scope
There was a problem hiding this comment.
can be declared as the if-init var to reduce its scope
Do you mean something like if (auto it = m.find(10); it != m.end())? I do not think it is a common code style in clang.
Merge activity
|
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
Current implement will cache `OptionsSource` for each path, it will create lots of copy of `OptionsSource` when project has deep nested folder structure. New implement use vector to store `OptionsSource` and only cache the index. It can reduce memory usage and avoid meaningless copy.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Current implement will cache
OptionsSourcefor each path, it will create lots of copy ofOptionsSourcewhen project has deep nested folder structure.New implement use vector to store
OptionsSourceand only cache the index. It can reduce memory usage and avoid meaningless copy.