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>
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..bf47b13 100644 --- a/src/gn/builder.cc +++ b/src/gn/builder.cc
@@ -236,6 +236,9 @@ for (auto* bad_record : bad_records) { depstring += "\n\"" + bad_record->label().GetUserVisibleName(true) + "\""; + if (records_resolving_on_worker_.contains(bad_record)) { + depstring += " (currently resolving on worker)"; + } } *err = Err(Location(), "", depstring); } else { @@ -497,6 +500,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 +509,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 +524,47 @@ 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); + bool already_existed = !records_resolving_on_worker_.add(record); + DCHECK(!already_existed); + + 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) { + records_resolving_on_worker_.erase(record); + 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..e4d996c 100644 --- a/src/gn/builder.h +++ b/src/gn/builder.h
@@ -13,6 +13,7 @@ #include "gn/builder_record_map.h" #include "gn/label.h" #include "gn/label_ptr.h" +#include "gn/pointer_set.h" #include "gn/unique_vector.h" class ActionValues; @@ -121,6 +122,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 @@ -144,6 +148,8 @@ ResolvedGeneratedCallback resolved_and_generated_callback_; + PointerSet<const BuilderRecord> records_resolving_on_worker_; + Builder(const Builder&) = delete; Builder& operator=(const Builder&) = delete; };
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..76d285c 100644 --- a/src/gn/scheduler.cc +++ b/src/gn/scheduler.cc
@@ -25,6 +25,16 @@ } 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())) { + return is_failed_; + } + + has_been_shutdown_ = false; + is_running_ = true; main_thread_run_loop_->Run(); bool local_is_failed; { @@ -35,6 +45,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_;