GN: Don't import a file more than once.
GN caches the results of imports so we don't have to load them more than once.
But if two BUILD files load the same import at the same time, there is a race
Previously GN would resolve the winner after the import ran on the assumption
that imports are fast and collisions are rare. But some imports run expensive
scripts meaning they take a long time and collisions are likely. The result is
that a file can get imported more than once.
This patch adds extra locking around the import to block other threads and
only do the import once.
Increase the minimum thread count for low-end systems to 8. This gives the fastest running time on an older MacBook.
Both the import change and the thread change independently speed up GN on Mac Chrome by 150-300ms, so actual improvement should be 300-600ms. On beefy Windows and Linux workstations, there seems to be no change in performance with these changes, although the import change should help some worst-case situations.
BUG=599892
Review-Url: https://codereview.chromium.org/1957483004
Cr-Original-Commit-Position: refs/heads/master@{#392353}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: a5b852cfbe6b6912ee6d1af145cbe9c090e40b42
diff --git a/tools/gn/import_manager.cc b/tools/gn/import_manager.cc
index 86cfd45..a1555b9 100644
--- a/tools/gn/import_manager.cc
+++ b/tools/gn/import_manager.cc
@@ -4,6 +4,7 @@
#include "tools/gn/import_manager.h"
+#include "tools/gn/err.h"
#include "tools/gn/parse_tree.h"
#include "tools/gn/scheduler.h"
#include "tools/gn/scope_per_file_provider.h"
@@ -41,7 +42,23 @@
return scope;
}
-} // namesapce
+} // namespace
+
+struct ImportManager::ImportInfo {
+ ImportInfo() {}
+ ~ImportInfo() {}
+
+ // This lock protects the unique_ptr. Once the scope is computed,
+ // it is const and can be accessed read-only outside of the lock.
+ base::Lock load_lock;
+
+ std::unique_ptr<const Scope> scope;
+
+ // The result of loading the import. If the load failed, the scope will be
+ // null but this will be set to error. In this case the thread should not
+ // attempt to load the file, even if the scope is null.
+ Err load_result;
+};
ImportManager::ImportManager() {
}
@@ -55,39 +72,42 @@
Err* err) {
// See if we have a cached import, but be careful to actually do the scope
// copying outside of the lock.
- const Scope* imported_scope = nullptr;
+ ImportInfo* import_info = nullptr;
{
- base::AutoLock lock(lock_);
- ImportMap::const_iterator found = imports_.find(file);
- if (found != imports_.end())
- imported_scope = found->second.get();
+ base::AutoLock lock(imports_lock_);
+ std::unique_ptr<ImportInfo>& info_ptr = imports_[file];
+ if (!info_ptr)
+ info_ptr.reset(new ImportInfo);
+
+ // Promote the ImportInfo to outside of the imports lock.
+ import_info = info_ptr.get();
}
- if (!imported_scope) {
- // Do a new import of the file.
- std::unique_ptr<Scope> new_imported_scope =
- UncachedImport(scope->settings(), file, node_for_err, err);
- if (!new_imported_scope)
- return false;
+ // Now use the per-import-file lock to block this thread if another thread
+ // is already processing the import.
+ const Scope* import_scope = nullptr;
+ {
+ base::AutoLock lock(import_info->load_lock);
- // We loaded the file outside the lock. This means that there could be a
- // race and the file was already loaded on a background thread. Recover
- // from this and use the existing one if that happens.
- {
- base::AutoLock lock(lock_);
- ImportMap::const_iterator found = imports_.find(file);
- if (found != imports_.end()) {
- imported_scope = found->second.get();
- } else {
- imported_scope = new_imported_scope.get();
- imports_[file] = std::move(new_imported_scope);
+ if (!import_info->scope) {
+ // Only load if the import hasn't already failed.
+ if (!import_info->load_result.has_error()) {
+ import_info->scope = UncachedImport(
+ scope->settings(), file, node_for_err, &import_info->load_result);
+ }
+ if (import_info->load_result.has_error()) {
+ *err = import_info->load_result;
+ return false;
}
}
+
+ // Promote the now-read-only scope to outside the load lock.
+ import_scope = import_info->scope.get();
}
Scope::MergeOptions options;
options.skip_private_vars = true;
options.mark_dest_used = true; // Don't require all imported values be used.
- return imported_scope->NonRecursiveMergeTo(scope, options, node_for_err,
- "import", err);
+ return import_scope->NonRecursiveMergeTo(scope, options, node_for_err,
+ "import", err);
}
diff --git a/tools/gn/import_manager.h b/tools/gn/import_manager.h
index bd47a2a..78bb613 100644
--- a/tools/gn/import_manager.h
+++ b/tools/gn/import_manager.h
@@ -31,10 +31,13 @@
Err* err);
private:
- base::Lock lock_;
+ struct ImportInfo;
+
+ // Protects access to imports_. Do not hold when actually executing imports.
+ base::Lock imports_lock_;
// Owning pointers to the scopes.
- typedef std::map<SourceFile, std::unique_ptr<const Scope>> ImportMap;
+ typedef std::map<SourceFile, std::unique_ptr<ImportInfo>> ImportMap;
ImportMap imports_;
DISALLOW_COPY_AND_ASSIGN(ImportManager);
diff --git a/tools/gn/scheduler.cc b/tools/gn/scheduler.cc
index a711df1..b2e2a00 100644
--- a/tools/gn/scheduler.cc
+++ b/tools/gn/scheduler.cc
@@ -54,10 +54,13 @@
//
// One less worker thread than the number of physical CPUs seems to be a
// good value, both theoretically and experimentally. But always use at
- // least three workers to prevent us from being too sensitive to I/O latency
+ // least some workers to prevent us from being too sensitive to I/O latency
// on low-end systems.
+ //
+ // The minimum thread count is based on measuring the optimal threads for the
+ // Chrome build on a several-year-old 4-core MacBook.
int num_cores = GetCPUCount() / 2; // Almost all CPUs now are hyperthreaded.
- return std::max(num_cores - 1, 3);
+ return std::max(num_cores - 1, 8);
}
} // namespace
