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>
diff --git a/src/gn/analyzer_unittest.cc b/src/gn/analyzer_unittest.cc
index 5952562..dfd1486 100644
--- a/src/gn/analyzer_unittest.cc
+++ b/src/gn/analyzer_unittest.cc
@@ -15,7 +15,6 @@
#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"
@@ -44,7 +43,7 @@
~MockLoader() override = default;
};
-class AnalyzerTest : public TestWithScheduler {
+class AnalyzerTest : public testing::Test {
public:
AnalyzerTest()
: loader_(new MockLoader),
@@ -91,8 +90,6 @@
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 bf47b13..1f3ecd8 100644
--- a/src/gn/builder.cc
+++ b/src/gn/builder.cc
@@ -236,9 +236,6 @@
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 {
@@ -500,7 +497,6 @@
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) ||
@@ -509,10 +505,6 @@
!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))
@@ -524,47 +516,11 @@
if (!ResolvePools(toolchain, err))
return false;
}
- 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->item()->OnResolved(err))
+ return false;
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 e4d996c..a9c01aa 100644
--- a/src/gn/builder.h
+++ b/src/gn/builder.h
@@ -13,7 +13,6 @@
#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;
@@ -122,9 +121,6 @@
// 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
@@ -148,8 +144,6 @@
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 59b58f8..59068a6 100644
--- a/src/gn/builder_unittest.cc
+++ b/src/gn/builder_unittest.cc
@@ -166,7 +166,6 @@
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.
@@ -254,7 +253,6 @@
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);
@@ -292,7 +290,6 @@
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));
@@ -308,7 +305,6 @@
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);
@@ -339,14 +335,12 @@
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 4a6e193..dd1a606 100644
--- a/src/gn/item.h
+++ b/src/gn/item.h
@@ -69,7 +69,6 @@
// 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 76d285c..6525f33 100644
--- a/src/gn/scheduler.cc
+++ b/src/gn/scheduler.cc
@@ -25,16 +25,6 @@
}
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;
{
@@ -45,7 +35,6 @@
// 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 b90814f..cf29fe8 100644
--- a/src/gn/scheduler.h
+++ b/src/gn/scheduler.h
@@ -140,9 +140,6 @@
// 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_;
