Header check: select files from the existing file map Build the list of files to check by filtering the complete file map. This avoids rebuilding a second map and repeating the work to merge each target's source, public and generated file metadata. Derive the targets for reachability precomputation from the selected files, so targets without eligible files do not get caches. Add a test for this selection. Chromium `gn gen --check`, medians of five round-robin rounds: Mac (M4 Max): 3.16 s -> 3.07 s GCE c4d-384: 5.96 s -> 5.80 s Intel 13900KS: 3.42 s -> 3.24 s CPU time (c4d-standard-384): User: 40.0 s -> 39.6 s (-1%) Sys: 53.0 s -> 45.4 s (-14%) Heap stats: Allocs (count): 55.16 M -> 54.24 M (-2%) Heap (total): 25.58 GB -> 24.93 GB (-3%) Heap (peak): 5.74 GB -> 5.47 GB (-5%) Change-Id: Iaa4971421bf860d9453dc4ebfa5fd65452a1980c Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26340 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Philipp Wollermann <philwo@google.com> Reviewed-by: Matt Stark <msta@google.com>
diff --git a/src/gn/header_checker.cc b/src/gn/header_checker.cc index 92d15c2..a6dbaae 100644 --- a/src/gn/header_checker.cc +++ b/src/gn/header_checker.cc
@@ -179,26 +179,22 @@ bool HeaderChecker::Run(const std::vector<const Target*>& to_check, bool force_check, std::vector<Violation>* violations) { - FileMap files_to_check; - for (auto* check : to_check) { - // This function will get called with all target types, but check only - // applies to binary targets. - if (check->IsBinary()) - AddTargetToFileMap(check, &files_to_check); - } + std::unordered_set<const Target*> to_check_set(to_check.begin(), + to_check.end()); + std::vector<FileInformation> files = FilesToCheck(to_check_set); WorkerPool pool; { ScopedTrace precompute_trace(TraceItem::TRACE_CHECK_HEADERS, "Precompute reachability"); - std::set<const Target*> targets_to_precompute; - for (const auto& file : files_to_check) { - for (const auto& target_info : file.second.targets) { - if (target_info.target->check_includes()) - targets_to_precompute.insert(target_info.target); + std::vector<const Target*> targets_to_precompute; + std::unordered_set<const Target*> seen; + for (const FileInformation& file : files) { + for (const TargetInfo& info : file.targets) { + if (seen.insert(info.target).second) + targets_to_precompute.push_back(info.target); } } - if (!targets_to_precompute.empty()) { task_count_.Increment(); for (const auto* target : targets_to_precompute) { @@ -225,7 +221,7 @@ } } - RunCheckOverFiles(files_to_check, force_check, &pool); + RunCheckOverFiles(files, &pool); if (violations_.empty()) return true; @@ -234,44 +230,51 @@ return false; } -void HeaderChecker::RunCheckOverFiles(const FileMap& files, - bool force_check, - WorkerPool* pool) { - task_count_.Increment(); +std::vector<HeaderChecker::FileInformation> HeaderChecker::FilesToCheck( + const std::unordered_set<const Target*>& to_check) const { + std::vector<FileInformation> files; + files.reserve(file_map_.size()); - for (const auto& file : files) { + for (const auto& entry : file_map_) { + const FileInformation& info = entry.second; // Only check C-like source files (RC files also have includes). - const SourceFile::Type type = file.second.file.GetType(); + const SourceFile::Type type = info.file.GetType(); if (type != SourceFile::SOURCE_CPP && type != SourceFile::SOURCE_H && type != SourceFile::SOURCE_C && type != SourceFile::SOURCE_M && type != SourceFile::SOURCE_MM && type != SourceFile::SOURCE_RC) continue; if (!check_generated_) { - // If any target marks it as generated, don't check it. We have to check - // file_map_, which includes all known files; files only includes those - // being checked. + // If any target marks it as generated, don't check it. bool is_generated = false; - auto found = file_map_.find(file.first); - DCHECK(found != file_map_.end()); - for (const auto& vect_i : found->second.targets) + for (const auto& vect_i : info.targets) is_generated |= vect_i.is_generated; if (is_generated) continue; } TargetVector targets_to_check; - for (const auto& vect_i : file.second.targets) { - if (vect_i.target->check_includes()) { + for (const auto& vect_i : info.targets) { + if (vect_i.target->IsBinary() && to_check.contains(vect_i.target) && + vect_i.target->check_includes()) { targets_to_check.push_back(vect_i); } } if (targets_to_check.empty()) continue; + files.push_back({info.file, std::move(targets_to_check)}); + } + return files; +} + +void HeaderChecker::RunCheckOverFiles(const std::vector<FileInformation>& files, + WorkerPool* pool) { + task_count_.Increment(); + + for (const FileInformation& file : files) { task_count_.Increment(); - pool->PostTask([this, targets = std::move(targets_to_check), - file = file.second.file]() { DoWork(targets, file); }); + pool->PostTask([this, &file]() { DoWork(file.targets, file.file); }); } if (!task_count_.Decrement()) {
diff --git a/src/gn/header_checker.h b/src/gn/header_checker.h index 98ab7f8..34941dd 100644 --- a/src/gn/header_checker.h +++ b/src/gn/header_checker.h
@@ -15,6 +15,7 @@ #include <shared_mutex> #include <string_view> #include <unordered_map> +#include <unordered_set> #include <vector> #include "base/atomic_ref_count.h" @@ -234,6 +235,8 @@ FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, CheckIncludeSwiftModule); FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, SourceFileForInclude); FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, + RunPrecomputesReachabilityOnlyForCheckedFiles); + FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, SourceFileForInclude_FileNotFound); FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, Friend); FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, CheckIncludesStrictTransitive); @@ -278,10 +281,14 @@ std::equal_to<>>; using PathExistsCallback = std::function<bool(const base::FilePath& path)>; - // Backend for Run() that takes the list of files to check. The errors_ list - // will be populate on failure. - void RunCheckOverFiles(const FileMap& files, - bool force_check, + // Collects the files of the given targets that need checking, each with the + // targets it is checked against. + std::vector<FileInformation> FilesToCheck( + const std::unordered_set<const Target*>& to_check) const; + + // Backend for Run() that checks the given files. The errors_ list will be + // populated on failure. + void RunCheckOverFiles(const std::vector<FileInformation>& files, WorkerPool* pool); void DoWork(const TargetVector& targets, const SourceFile& file);
diff --git a/src/gn/header_checker_unittest.cc b/src/gn/header_checker_unittest.cc index 04525b5..a8ea4a0 100644 --- a/src/gn/header_checker_unittest.cc +++ b/src/gn/header_checker_unittest.cc
@@ -355,6 +355,26 @@ EXPECT_GT(errors.size(), 0); } +TEST_F(HeaderCheckerTest, RunPrecomputesReachabilityOnlyForCheckedFiles) { + a_.sources().push_back(SourceFile("//out/Debug/generated.cc")); + + auto checker = CreateChecker(); + std::vector<HeaderChecker::Violation> violations; + EXPECT_TRUE(checker->Run(targets_, false, &violations)); + + auto has_cache = [&checker](const Target* target) { + for (const auto& shard : checker->dependency_cache_) { + if (shard.cache.contains(target)) + return true; + } + return false; + }; + EXPECT_TRUE(has_cache(&a_)); + EXPECT_FALSE(has_cache(&b_)); + EXPECT_FALSE(has_cache(&c_)); + EXPECT_FALSE(has_cache(&d_)); +} + TEST_F(HeaderCheckerTest, SourceFileForInclude) { using base::FilePath; const std::vector<SourceDir> kIncludeDirs = {