Reland #2 "Run Target::OnResolve() on background threads"

This reverts commit 54169531ed6da64425ad6e8e9535945c8f6220d0.

Reason for reland: Removed the PointerSet that was triggering an
infinite loop in HashTableBase::NodeLookup(). I'll fix that bug
separately.

Bug: chromium:398002893
Original change's description:
> Revert "Reland "Run Target::OnResolve() on background threads""
>
> This reverts commit 0fcc64f7d006474a026d8f78f3a95dac84cd97ea.
>
> Reason for revert: Failing to roll into chromium:
> https://chromium-review.googlesource.com/c/chromium/src/+/6641508
>
> Bug: chromium:398002893
> Original change's description:
> > Reland "Run Target::OnResolve() on background threads"
> >
> > This reverts commit 8ea3af9118a65387fe4d8428db6f59c5927dd55a.
> >
> > Reason for reland: Fixed test
> >
> > Bug: chromium:398002893
> > Original change's description:
> > > Revert "Run Target::OnResolve() on background threads"
> > >
> > > This reverts commit 3ef7da38dcfb63212d97a987d5c60374134e1007.
> > >
> > > Reason for revert: Failing tests /w asan (and on release bots)
> > >
> > > Bug: chromium:398002893
> > > Original change's description:
> > > > Run Target::OnResolve() on background threads
> > > >
> > > > OnResolve() is often trivial, but it can also not be, as there are a lot
> > > > of possible checks that it can do.
> > > >
> > > > "gn --tracelog gen" now shows all threads being always busy.
> > > >
> > > > Hyperfine Benchmark from Chromium:
> > > > Before:
> > > >   Time (mean ± σ):      7.349 s ±  0.251 s    [User: 66.903 s, System: 50.486 s]
> > > >   Range (min … max):    6.974 s …  7.655 s    10 runs
> > > >
> > > > After:
> > > >   Time (mean ± σ):      6.563 s ±  0.313 s    [User: 67.873 s, System: 53.023 s]
> > > >   Range (min … max):    6.186 s …  7.281 s    10 runs
> > > > Bug: chromium:398002893
> > > > Change-Id: I30bd9b7541a961b1d0cef80c229b0a3f24752d3e
> > > > Reviewed-on: https://gn-review.googlesource.com/c/gn/+/19020
> > > > Reviewed-by: David Turner <digit@google.com>
> > > > Commit-Queue: David Turner <digit@google.com>
> > > > Reviewed-by: Takuto Ikuta <tikuta@google.com>
> > >
> > > # Not skipping CQ checks because original CL landed > 1 day ago.
> > >
> > > Bug: chromium:398002893
> > > Change-Id: I4ca499e210288b46a91209967a635004c9d0ad68
> > > Reviewed-on: https://gn-review.googlesource.com/c/gn/+/19060
> > > Reviewed-by: Andrew Grieve <agrieve@google.com>
> > > Commit-Queue: Roland McGrath <mcgrathr@google.com>
> > > Reviewed-by: Roland McGrath <mcgrathr@google.com>
> >
> > # Not skipping CQ checks because this is a reland.
> >
> > Bug: chromium:398002893
> > Change-Id: Ia052c4fb4182bf879e418effcf17f27868131be2
> > Reviewed-on: https://gn-review.googlesource.com/c/gn/+/19080
> > Commit-Queue: Andrew Grieve <agrieve@google.com>
> > Reviewed-by: David Turner <digit@google.com>
>
> TBR=mcgrathr@google.com,tikuta@google.com,agrieve@google.com,digit@google.com,gn-scoped@luci-project-accounts.iam.gserviceaccount.com
>
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: chromium:398002893
> Change-Id: I85d6f9b25463afddb8e9c31dd7568170c12c0232
> Reviewed-on: https://gn-review.googlesource.com/c/gn/+/19120
> Commit-Queue: Andrew Grieve <agrieve@google.com>
> Reviewed-by: Andrew Grieve <agrieve@google.com>
> Reviewed-by: David Turner <digit@google.com>

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:398002893
Change-Id: I7122869f620b2a9e8e9c8d5a224ee46431e3c9c2
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/19140
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Andrew Grieve <agrieve@google.com>
Reviewed-by: David Turner <digit@google.com>
diff --git a/src/gn/analyzer_unittest.cc b/src/gn/analyzer_unittest.cc
index dfd1486..5952562 100644
--- a/src/gn/analyzer_unittest.cc
+++ b/src/gn/analyzer_unittest.cc
@@ -15,6 +15,7 @@
 #include "gn/source_file.h"
 #include "gn/substitution_list.h"
 #include "gn/target.h"
+#include "gn/test_with_scheduler.h"
 #include "gn/tool.h"
 #include "gn/toolchain.h"
 #include "util/test/test.h"
@@ -43,7 +44,7 @@
   ~MockLoader() override = default;
 };
 
-class AnalyzerTest : public testing::Test {
+class AnalyzerTest : public TestWithScheduler {
  public:
   AnalyzerTest()
       : loader_(new MockLoader),
@@ -90,6 +91,8 @@
 
   void RunAnalyzerTest(const std::string& input,
                        const std::string& expected_output) {
+    // Wait for any async OnResolve() to happen.
+    scheduler().Run();
     Analyzer analyzer(builder_, SourceFile("//build/config/BUILDCONFIG.gn"),
                       SourceFile("//.gn"),
                       {SourceFile("//out/debug/args.gn"),
diff --git a/src/gn/builder.cc b/src/gn/builder.cc
index 1f3ecd8..17f5101 100644
--- a/src/gn/builder.cc
+++ b/src/gn/builder.cc
@@ -497,6 +497,7 @@
 
   if (record->type() == BuilderRecord::ITEM_TARGET) {
     Target* target = record->item()->AsTarget();
+    // All deps must be resolved before OnResolved can be called.
     if (!ResolveDeps(&target->public_deps(), err) ||
         !ResolveDeps(&target->private_deps(), err) ||
         !ResolveDeps(&target->data_deps(), err) ||
@@ -505,6 +506,10 @@
         !ResolveConfigs(&target->public_configs(), err) ||
         !ResolvePool(target, err) || !ResolveToolchain(target, err))
       return false;
+
+    // Offload Target::OnResolved to a worker thread.
+    ScheduleTargetOnResolve(record);
+    return true;
   } else if (record->type() == BuilderRecord::ITEM_CONFIG) {
     Config* config = record->item()->AsConfig();
     if (!ResolveConfigs(&config->configs(), err))
@@ -516,11 +521,44 @@
     if (!ResolvePools(toolchain, err))
       return false;
   }
-
-  record->set_resolved(true);
-
   if (!record->item()->OnResolved(err))
     return false;
+
+  return CompleteItemResolution(record, err);
+}
+
+void Builder::ScheduleTargetOnResolve(BuilderRecord* record) {
+  DCHECK(g_scheduler);
+
+  g_scheduler->IncrementWorkCount();
+  g_scheduler->ScheduleWork([this, 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);
+    }
+  }
+  g_scheduler->DecrementWorkCount();
+}
+
+bool Builder::CompleteItemResolution(BuilderRecord* record, Err* err) {
+  record->set_resolved(true);
+
   if (record->should_generate() && resolved_and_generated_callback_)
     resolved_and_generated_callback_(record);
 
diff --git a/src/gn/builder.h b/src/gn/builder.h
index a9c01aa..6776363 100644
--- a/src/gn/builder.h
+++ b/src/gn/builder.h
@@ -121,6 +121,9 @@
   // 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);
+  bool CompleteItemResolution(BuilderRecord* record, Err* err);
 
   // Fills in the pointers in the given vector based on the labels. We assume
   // that everything should be resolved by this point, so will return an error
diff --git a/src/gn/builder_unittest.cc b/src/gn/builder_unittest.cc
index 59068a6..59b58f8 100644
--- a/src/gn/builder_unittest.cc
+++ b/src/gn/builder_unittest.cc
@@ -166,6 +166,7 @@
   b->set_output_type(Target::SHARED_LIBRARY);
   b->visibility().SetPublic();
   builder_.ItemDefined(std::unique_ptr<Item>(b));
+  scheduler().Run();
 
   // B depends only on the already-loaded C and toolchain so we shouldn't have
   // requested anything else.
@@ -253,6 +254,7 @@
   a->public_deps().push_back(LabelTargetPair(b_label));
   a->set_output_type(Target::EXECUTABLE);
   builder_.ItemDefined(std::unique_ptr<Item>(a));
+  scheduler().Run();
 
   // A should have the generate bit set since it's in the default toolchain.
   BuilderRecord* a_record = builder_.GetRecord(a_label);
@@ -290,6 +292,7 @@
 
   Target* b = new Target(&settings2, b_label);
   b->set_output_type(Target::EXECUTABLE);
+  b->visibility().SetPublic();  // Allow 'a' to depend on 'b'
   b->gen_deps().push_back(LabelTargetPair(c_label));
   builder_.ItemDefined(std::unique_ptr<Item>(b));
 
@@ -305,6 +308,7 @@
   Target* d = new Target(&settings2, d_label);
   d->set_output_type(Target::EXECUTABLE);
   builder_.ItemDefined(std::unique_ptr<Item>(d));
+  scheduler().Run();
 
   BuilderRecord* a_record = builder_.GetRecord(a_label);
   BuilderRecord* b_record = builder_.GetRecord(b_label);
@@ -335,12 +339,14 @@
   Target* a = new Target(&settings_, a_label);
   a->gen_deps().push_back(LabelTargetPair(b_label));
   a->set_output_type(Target::EXECUTABLE);
+  a->visibility().SetPublic(); // Allow 'b' to depend on 'a'
   builder_.ItemDefined(std::unique_ptr<Item>(a));
 
   Target* b = new Target(&settings2, b_label);
   b->private_deps().push_back(LabelTargetPair(a_label));
   b->set_output_type(Target::EXECUTABLE);
   builder_.ItemDefined(std::unique_ptr<Item>(b));
+  scheduler().Run();
 
   Err err;
   EXPECT_TRUE(builder_.CheckForBadItems(&err));
diff --git a/src/gn/item.h b/src/gn/item.h
index dd1a606..4a6e193 100644
--- a/src/gn/item.h
+++ b/src/gn/item.h
@@ -69,6 +69,7 @@
   // Called when this item is resolved, meaning it and all of its dependents
   // have no unresolved deps. Returns true on success. Sets the error and
   // returns false on failure.
+  // Can be called from any thread, so must not mutate dependencies.
   virtual bool OnResolved(Err* err);
 
  private:
diff --git a/src/gn/scheduler.cc b/src/gn/scheduler.cc
index 6525f33..9265b51 100644
--- a/src/gn/scheduler.cc
+++ b/src/gn/scheduler.cc
@@ -25,6 +25,19 @@
 }
 
 bool Scheduler::Run() {
+  // Reentrancy check.
+  CHECK(!is_running_);
+
+  // Ensure there is at least one task, (or else this will wait forever).
+  if (is_failed_ || (work_count_.IsZero() && pool_work_count_.IsZero())) {
+     // Flush any posted tasks (that were posted from the UI thread).
+     main_thread_run_loop_->PostQuit();
+     main_thread_run_loop_->Run();
+     return is_failed_;
+  }
+
+  has_been_shutdown_ = false;
+  is_running_ = true;
   main_thread_run_loop_->Run();
   bool local_is_failed;
   {
@@ -35,6 +48,7 @@
   // Don't do this while holding |lock_|, since it will block on the workers,
   // which may be in turn waiting on the lock.
   WaitForPoolTasks();
+  is_running_ = false;
   return !local_is_failed;
 }
 
diff --git a/src/gn/scheduler.h b/src/gn/scheduler.h
index cf29fe8..b90814f 100644
--- a/src/gn/scheduler.h
+++ b/src/gn/scheduler.h
@@ -140,6 +140,9 @@
   // loop.
   bool has_been_shutdown_ = false;
 
+  // Prevent reentrancy.
+  bool is_running_ = false;
+
   // Protected by the lock. See the corresponding Add/Get functions above.
   std::vector<base::FilePath> gen_dependencies_;
   std::vector<SourceFile> written_files_;