Header check: batch reachability and file checking tasks Post reachability precomputation in groups of 32 targets and file checking in groups of 64 files. This reduces queue operations and completion counter updates. Collect violations within each group and merge them under one lock. Share the task posting and completion wait between both phases. Chromium `gn gen --check`, medians of five round-robin rounds: Mac (M4 Max): 3.07 s -> 3.13 s (within noise) GCE c4d-384: 5.80 s -> 5.15 s Intel 13900KS: 3.24 s -> 3.30 s (within noise) CPU time (c4d-standard-384): User: 39.6 s -> 38.5 s (-3%) Sys: 45.4 s -> 50.2 s (+11%) Heap stats: Allocs (count): 54.24 M -> 54.24 M (0%) Heap (total): 24.93 GB -> 24.92 GB (0%) Heap (peak): 5.47 GB -> 5.46 GB (0%) Change-Id: I8b884c5dd5c1bd58a40aaf151073ce9058326501 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26341 Reviewed-by: Takuto Ikuta <tikuta@google.com> Reviewed-by: Matt Stark <msta@google.com> Commit-Queue: Philipp Wollermann <philwo@google.com>
diff --git a/src/gn/header_checker.cc b/src/gn/header_checker.cc index a6dbaae..84b4433 100644 --- a/src/gn/header_checker.cc +++ b/src/gn/header_checker.cc
@@ -5,7 +5,11 @@ #include "gn/header_checker.h" #include <algorithm> +#include <condition_variable> +#include <mutex> +#include <span> +#include "base/atomic_ref_count.h" #include "base/containers/queue.h" #include "base/files/file_util.h" #include "base/strings/string_util.h" @@ -159,6 +163,47 @@ is_marked_friend->label()); } +// Runs work on the items in chunks of chunk_size on the pool and waits for +// all chunks to complete. +template <typename T, typename Work> +void DoWorkChunked(WorkerPool& pool, + const std::vector<T>& items, + size_t chunk_size, + const Work& work) { + base::AtomicRefCount task_count; + std::mutex task_count_lock; + std::condition_variable task_count_cv; + auto finish_task = [&]() { + // Hold the lock across the decrement so the waiter cannot return and + // destroy the completion state before the notification is done. + std::unique_lock<std::mutex> lock(task_count_lock); + if (!task_count.Decrement()) { + // Signal |task_count_cv| when |task_count| becomes zero. + task_count_cv.notify_one(); + } + }; + + // Hold one extra reference while posting so the count can't reach zero + // before all chunks are posted. + task_count.Increment(); + std::span<const T> all(items); + for (size_t begin = 0; begin < all.size(); begin += chunk_size) { + std::span<const T> chunk = + all.subspan(begin, std::min(chunk_size, all.size() - begin)); + task_count.Increment(); + pool.PostTask([&work, &finish_task, chunk]() { + work(chunk); + finish_task(); + }); + } + finish_task(); + + // Wait for all tasks posted by this function to complete. + std::unique_lock<std::mutex> lock(task_count_lock); + while (!task_count.IsZero()) + task_count_cv.wait(lock); +} + } // namespace HeaderChecker::HeaderChecker(const BuildSettings* build_settings, @@ -168,8 +213,7 @@ : build_settings_(build_settings), check_generated_(check_generated), check_system_(check_system), - errors_lock_(), - task_count_cv_() { + errors_lock_() { for (auto* target : targets) AddTargetToFileMap(target, &file_map_); } @@ -195,33 +239,18 @@ targets_to_precompute.push_back(info.target); } } - if (!targets_to_precompute.empty()) { - task_count_.Increment(); - for (const auto* target : targets_to_precompute) { - task_count_.Increment(); - pool.PostTask([this, target]() { - ReachabilityCache& cache = GetReachabilityCacheForTarget(target); - cache.PerformDependencyWalk(true); - cache.PerformDependencyWalk(false); - if (!task_count_.Decrement()) { - std::unique_lock<std::mutex> lock(task_count_lock_); - task_count_cv_.notify_one(); - } - }); - } - - if (!task_count_.Decrement()) { - std::unique_lock<std::mutex> lock(task_count_lock_); - task_count_cv_.notify_one(); - } - - std::unique_lock<std::mutex> lock(task_count_lock_); - while (!task_count_.IsZero()) - task_count_cv_.wait(lock); - } + DoWorkChunked(pool, targets_to_precompute, 32, + [this](std::span<const Target* const> chunk) { + for (const Target* target : chunk) { + ReachabilityCache& cache = + GetReachabilityCacheForTarget(target); + cache.PerformDependencyWalk(true); + cache.PerformDependencyWalk(false); + } + }); } - RunCheckOverFiles(files, &pool); + RunCheckOverFiles(files, pool); if (violations_.empty()) return true; @@ -269,40 +298,19 @@ } void HeaderChecker::RunCheckOverFiles(const std::vector<FileInformation>& files, - WorkerPool* pool) { - task_count_.Increment(); - - for (const FileInformation& file : files) { - task_count_.Increment(); - pool->PostTask([this, &file]() { DoWork(file.targets, file.file); }); - } - - if (!task_count_.Decrement()) { - std::unique_lock<std::mutex> lock(task_count_lock_); - task_count_cv_.notify_one(); - } - - // Wait for all tasks posted by this method to complete. - std::unique_lock<std::mutex> auto_lock(task_count_lock_); - while (!task_count_.IsZero()) - task_count_cv_.wait(auto_lock); -} - -void HeaderChecker::DoWork(const TargetVector& targets, - const SourceFile& file) { - std::vector<Violation> violations; - if (!CheckFile(targets, file, &violations)) { - std::lock_guard<std::mutex> lock(errors_lock_); - violations_.insert(violations_.end(), - std::make_move_iterator(violations.begin()), - std::make_move_iterator(violations.end())); - } - - if (!task_count_.Decrement()) { - // Signal |task_count_cv_| when |task_count_| becomes zero. - std::unique_lock<std::mutex> auto_lock(task_count_lock_); - task_count_cv_.notify_one(); - } + WorkerPool& pool) { + DoWorkChunked( + pool, files, 64, [this](std::span<const FileInformation> chunk) { + std::vector<Violation> local_violations; + for (const FileInformation& file : chunk) + CheckFile(file.targets, file.file, &local_violations); + if (local_violations.empty()) + return; + std::lock_guard<std::mutex> lock(errors_lock_); + violations_.insert(violations_.end(), + std::make_move_iterator(local_violations.begin()), + std::make_move_iterator(local_violations.end())); + }); } // static
diff --git a/src/gn/header_checker.h b/src/gn/header_checker.h index 34941dd..e014d4a 100644 --- a/src/gn/header_checker.h +++ b/src/gn/header_checker.h
@@ -7,7 +7,6 @@ #include <array> #include <atomic> -#include <condition_variable> #include <functional> #include <map> #include <mutex> @@ -18,7 +17,6 @@ #include <unordered_set> #include <vector> -#include "base/atomic_ref_count.h" #include "base/gtest_prod_util.h" #include "base/memory/ref_counted.h" #include "gn/c_include_iterator.h" @@ -289,9 +287,7 @@ // 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); + WorkerPool& pool); // Adds the sources and public files from the given target to the given map. static void AddTargetToFileMap(const Target* target, FileMap* dest); @@ -363,10 +359,6 @@ // Maps source files to targets it appears in (usually just one target). FileMap file_map_; - // Number of tasks posted by RunCheckOverFiles() that haven't completed their - // execution. - base::AtomicRefCount task_count_; - static constexpr size_t kNumShards = 64; struct DependencyCacheShard { mutable std::shared_mutex lock; @@ -386,13 +378,6 @@ // Returns the cache for the given target, creating it if it doesn't exist. ReachabilityCache& GetReachabilityCacheForTarget(const Target* target) const; - // Separate lock for task count synchronization since std::condition_variable - // only works with std::unique_lock<std::mutex>. - std::mutex task_count_lock_; - - // Signaled when |task_count_| becomes zero. - std::condition_variable task_count_cv_; - HeaderChecker(const HeaderChecker&) = delete; HeaderChecker& operator=(const HeaderChecker&) = delete; };