Only run target checks in background threads during resolution. Before this CL, Target::OnResolved() did propagate information from dependencies to finalize the instance's state, and would also run several checks to report errors. The CL separates these into Target::OnResolvedWithoutChecks() and Target::RunChecksAfterResolution(), then modifies the Builder::ResolveItem() implementation to only call the latter in background threads, while keeping the call to the former in the main thread. This ensures that the result of resolution is deterministic, thus removing a source of data races that masked the problems described in the associated bug. This CL does *not* fix the issue though, this will happen in a future CL. Benchmarking shows no statistical difference in `gn gen` and `gn gen --check` times for a Chrome/Android build configuration. Bug: 494481832 Change-Id: I3947709fbda2fb39ccc75c620f8b9d535e0d3d26 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/21720 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: David Turner <digit@google.com> Reviewed-by: Andrew Grieve <agrieve@google.com>

diff --git a/src/gn/builder.cc b/src/gn/builder.cc
index 90e3f5b..9a85e20 100644
--- a/src/gn/builder.cc
+++ b/src/gn/builder.cc

@@ -535,9 +535,12 @@
         !ResolvePool(target, err) || !ResolveToolchain(target, err))
       return false;
 
-    // Offload Target::OnResolved to a worker thread.
-    ScheduleTargetOnResolve(record);
-    return true;
+    if (!target->OnResolvedWithoutChecks(err))
+      return false;
+
+    // Offload Target::RunChecksAfterResolution to a worker thread.
+    ScheduleBackgroundTargetChecks(record);
+    return CompleteItemResolution(record, err);
   } else if (record->type() == BuilderRecord::ITEM_CONFIG) {
     Config* config = record->item()->AsConfig();
     if (!ResolveConfigs(&config->configs(), err))
@@ -555,31 +558,17 @@
   return CompleteItemResolution(record, err);
 }
 
-void Builder::ScheduleTargetOnResolve(BuilderRecord* record) {
+void Builder::ScheduleBackgroundTargetChecks(BuilderRecord* record) {
   DCHECK(g_scheduler);
 
   g_scheduler->IncrementWorkCount();
-  g_scheduler->ScheduleWork([this, record]() {
+  g_scheduler->ScheduleWork([record]() {
     Err err;
-    bool success = record->item()->AsTarget()->OnResolved(&err);
-    DCHECK(success == !err.has_error());
-
-    g_scheduler->task_runner()->PostTask(
-        [this, record, err]() { CompleteAsyncTargetResolution(record, err); });
-  });
-}
-
-void Builder::CompleteAsyncTargetResolution(BuilderRecord* record,
-                                            const Err& err) {
-  if (err.has_error()) {
-    g_scheduler->FailWithError(err);
-  } else {
-    Err next_err;
-    if (!CompleteItemResolution(record, &next_err)) {
-      g_scheduler->FailWithError(next_err);
+    if (!record->item()->AsTarget()->RunChecksAfterResolution(&err)) {
+      g_scheduler->FailWithError(err);
     }
-  }
-  g_scheduler->DecrementWorkCount();
+    g_scheduler->DecrementWorkCount();
+  });
 }
 
 bool Builder::CompleteItemResolution(BuilderRecord* record, Err* err) {

diff --git a/src/gn/builder.h b/src/gn/builder.h
index 927de0a..ebff0cc 100644
--- a/src/gn/builder.h
+++ b/src/gn/builder.h

@@ -124,8 +124,7 @@
   // This takes a BuilderRecord with resolved dependencies, and fills in the
   // target's Label*Vectors with the resolved pointers.
   bool ResolveItem(BuilderRecord* record, Err* err);
-  void ScheduleTargetOnResolve(BuilderRecord* record);
-  void CompleteAsyncTargetResolution(BuilderRecord* record, const Err& err);
+  void ScheduleBackgroundTargetChecks(BuilderRecord* record);
   bool CompleteItemResolution(BuilderRecord* record, Err* err);
 
   // Fills in the pointers in the given vector based on the labels. We assume

diff --git a/src/gn/builder_unittest.cc b/src/gn/builder_unittest.cc
index 8279061..68f365e 100644
--- a/src/gn/builder_unittest.cc
+++ b/src/gn/builder_unittest.cc

@@ -527,6 +527,11 @@
   Label b_label(SourceDir("//b/"), "b", toolchain_dir, toolchain_name);
   Label c_label(SourceDir("//c/"), "c", toolchain_dir, toolchain_name);
 
+  // Track written targets.
+  std::vector<const BuilderRecord*> written;
+  builder_.set_resolved_and_generated_callback(
+      [&written](const BuilderRecord* record) { written.push_back(record); });
+
   // Define A. A depends on B and has validation C.
   auto a = std::make_unique<Target>(&settings_, a_label);
   a->set_output_type(Target::ACTION);
@@ -541,11 +546,6 @@
   c->visibility().SetPublic();
   builder_.ItemDefined(std::move(c));
 
-  // Track written targets.
-  std::vector<const BuilderRecord*> written;
-  builder_.set_resolved_and_generated_callback(
-      [&written](const BuilderRecord* record) { written.push_back(record); });
-
   // C should resolve. A is waiting on B.
   scheduler().Run();
 
@@ -596,6 +596,11 @@
   Label c_label(SourceDir("//c/"), "c", toolchain_dir, toolchain_name);
   Label e_label(SourceDir("//e/"), "e", toolchain_dir, toolchain_name);
 
+  // Track written targets.
+  std::vector<const BuilderRecord*> written;
+  builder_.set_resolved_and_generated_callback(
+      [&written](const BuilderRecord* record) { written.push_back(record); });
+
   // Define A with validation B.
   auto a = std::make_unique<Target>(&settings_, a_label);
   a->set_output_type(Target::GROUP);
@@ -612,11 +617,6 @@
   builder_.ItemDefined(std::move(b));
   BuilderRecord* b_record = builder_.GetRecord(b_label);
 
-  // Track written targets.
-  std::vector<const BuilderRecord*> written;
-  builder_.set_resolved_and_generated_callback(
-      [&written](const BuilderRecord* record) { written.push_back(record); });
-
   // A resolves (validations don't block resolution).
   // B waits for E.
   scheduler().Run();

diff --git a/src/gn/target.cc b/src/gn/target.cc
index 062beba..b66c636 100644
--- a/src/gn/target.cc
+++ b/src/gn/target.cc

@@ -566,6 +566,10 @@
 }
 
 bool Target::OnResolved(Err* err) {
+  return OnResolvedWithoutChecks(err) && RunChecksAfterResolution(err);
+}
+
+bool Target::OnResolvedWithoutChecks(Err* err) {
   DCHECK(output_type_ != UNKNOWN);
   DCHECK(toolchain_) << "Toolchain should have been set before resolving.";
 
@@ -608,16 +612,6 @@
   if (!SwiftValues::OnTargetResolved(this, err))
     return false;
 
-  if (!CheckSourceSetLanguages(err))
-    return false;
-  if (!CheckVisibility(err))
-    return false;
-  if (!CheckTestonly(err))
-    return false;
-  if (!CheckAssertNoDeps(err))
-    return false;
-  CheckSourcesGenerated();
-
   if (!write_runtime_deps_output_.value().empty())
     g_scheduler->AddWriteRuntimeDepsTarget(this);
 
@@ -630,6 +624,19 @@
   return true;
 }
 
+bool Target::RunChecksAfterResolution(Err* err) {
+  if (!CheckSourceSetLanguages(err))
+    return false;
+  if (!CheckVisibility(err))
+    return false;
+  if (!CheckTestonly(err))
+    return false;
+  if (!CheckAssertNoDeps(err))
+    return false;
+  CheckSourcesGenerated();
+  return true;
+}
+
 bool Target::IsBinary() const {
   return output_type_ == EXECUTABLE || output_type_ == SHARED_LIBRARY ||
          output_type_ == LOADABLE_MODULE || output_type_ == STATIC_LIBRARY ||

diff --git a/src/gn/target.h b/src/gn/target.h
index ccd5372..eb9e0b9 100644
--- a/src/gn/target.h
+++ b/src/gn/target.h

@@ -75,8 +75,22 @@
   // Item overrides.
   Target* AsTarget() override;
   const Target* AsTarget() const override;
+
+  // NOTE: This calls OnResolvedWithoutChecks followed by RunChecksAfterResolution.
   bool OnResolved(Err* err) override;
 
+  // Perform all resolution steps except for checks. When this function
+  // returns, the content of the Target instance, as well as all its
+  // standard dependencies (i.e. without validations and gen_deps) are
+  // considered final and immutable. This allows running checks in background
+  // threads while the rest of the resolution algorithm continues in the main
+  // thread.
+  bool OnResolvedWithoutChecks(Err* err);
+
+  // Run checks. This requires OnResolvedWithoutChecks() to have been called
+  // first. This can run in a background thread.
+  bool RunChecksAfterResolution(Err* err);
+
   OutputType output_type() const { return output_type_; }
   void set_output_type(OutputType t) { output_type_ = t; }