tree cadff450fe5e0c3769df302a93379fc5a7bc2ba3
parent 755ea88213fc46818ede21a28fc2c191e2a24bcb
author David 'Digit' Turner <digit@google.com> 1774023627 +0000
committer GN LUCI <gn-scoped@luci-project-accounts.iam.gserviceaccount.com> 1774282735 -0700

Remove premature target writing before validation during `gn gen`.

This change modifies the way GN schedules writing targets to the
Ninja build plan to remove random crashes that have been affecting
the Fuchsia build for several weeks now.

The root cause of the problem, described in the associated bug,
is that a target and *all* its dependencies, including validations,
must be resolved to ensure there are no crash during the write
(which can call Target::GetMetadata() for generated_file() targets,
which will walk standard and validation targets).

Before this CL, the code used the condition that a target had to
be resolved, and all its validations *defined*, which is NOT
sufficient.

The crash was hidden by the fact that target resolution was pushed
to the background, resulting in race conditions and non-deterministic
ordering of the writes. This behavior was removed in a previous CL,
the current one focuses on the actual logic fix.

The fix includes defining a new RecordState enum to better describe
the different states of a given BuilderRecord, replacing the simple
`resolved_` boolean flag. State transitions can only happen under
specific conditions that are now properly documented.

- builder_record.h: Introduce RecordState enum to model the
  state of each record (Init/Defined/Resolved/Finalized). and
  document them properly, along with the conditions when a
  record can change state.

  Rename resolved() to is_resolved(), can_write() to can_finalize(),
  add is_defined() and is_finalized()

  Add Set{Defined,Resolved,Finalized}() methods to change the
  state and document what the caller should do just after that
  to ensure proper tracking of state changes.

  Add NotifyWaitingDependentsOnXXX() template methods to
  clarify the builder.cc code and keep the BuilderRecordSet
  values hidden.

  Remove validation_deps_ which is no longer useful.

  Rename the BuilderRecordSet variables for clarity, better
  explaining their purpose.

  Add DEBUG_BUILDER_RECORD macro. Set this to 1 to enable
  debug logs during Builder operations during development.

- builder.cc: Add FinalizeItem() method, and modify the logic
  accordingly, using the new BuilderRecord methods. Add
  DEBUG_BUILDER_RECORD_LOG() statements to print debug traces
  when DEBUG_BUILD_RECORD is set to 1.

- builder_unittest.cc: Adjust unit-tests to reflect the new
  logic.

Benchmarking shows now statistically difference in `gn gen`
and `gn gen --check` times for Fuchsia and Chrome builds on
a local cloudtop.

Bug: 494481832
Change-Id: Ibeae5350818c6b73fb88247fc73f85ab596c9531
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/21721
Commit-Queue: David Turner <digit@google.com>
Reviewed-by: Andrew Grieve <agrieve@google.com>