Rename HeaderChecker::IsFileInOuputDir to IsFileInOutputDir, and the
TargetTest.RuntimeOuputs and TargetTest.RustRuntimeOuputs tests to
*RuntimeOutputs. No behavior change.
Change-Id: I13ea5fe4a3ecc8e322ba9132855b9b616405bab1
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27320
Commit-Queue: Matt Stark <msta@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Matt Stark <msta@google.com>
A "gn check --check-generated" failure in a generated source can only be
suppressed with no_check_targets or check_includes = false, both of
which also skip the target's handwritten files.
no_check_generated_targets is a list of label patterns in .gn, in the
same format as no_check_targets. With --check-generated, gn check skips
the generated files (files in the output directory) of matching targets
and still checks their other files. Like check_targets and
no_check_targets, the list is not used when a label pattern is given on
the command line. gn check results are unchanged if the list is unset.
Bug: 559888973
Change-Id: I500cb396ca271daa69a407ea333e56438a28d6f4
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27220
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Matt Stark <msta@google.com>
Commit-Queue: Ryosuke Miyake <rysk@google.com>
Previously, if you had a target //skia:headers, which is part of the
group //skia:skia, which is in turn part of the group //:gn_all, and
you tried to include it from //foo:bar, it would see that // is a
prefix of //foo, and thus assume //:gn_all was a local alias.
Change-Id: Ib3fe2edce0d15d5228eb53f97964e2c66a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27300
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
Whenever a commit is created or squashed in git/jj, the commit hash of
HEAD changes, which previously updated last_commit_position.h during
gen.py and forced a rebuild of gn_main.cc and relinking of gn.
In debug mode, generate a dummy last_commit_position.h instead so that
squashes do not invalidate the build cache. Real commit positions
continue to be generated for release builds.
Change-Id: Ib9e2fac8c1ec74f04fb66f382629e1dc6a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27280
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
Note that this change is backwards-incompatible and will require
downstream projects to explicitly disable check_includes_strict in their
BUILDCONFIG.gn.
Bug: 555455099
Change-Id: Ie05cd7f182053355cba533a3c18ea0d06a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27240
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
Configure include sorting options in .clang-format based on
Chromium's settings to adhere to Google C++ Style Guide:
- IncludeBlocks: Regroup
- IncludeCategories (with special priorities for Windows headers)
- IncludeIsMainRegex (for recognizing main headers in tests and platform files)
This ensures that tools/run_formatter.sh automatically sorts headers
and groups them appropriately (e.g. separating main headers, C system headers,
and C++ standard library headers with blank lines).
Also reformat existing files with tools/run_formatter.sh.
Change-Id: I1a82e23a7e1ac117a5f13d43a611a13c475fe2d1
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27200
Reviewed-by: David Turner <digit@google.com>
Commit-Queue: Takuto Ikuta <tikuta@google.com>
When a generated_file() target is written to the Ninja build
plan, delay its output file write if `collect_validations_metadata`
is true. Instead, the writes will be performed after all targets
have been resolved to ensure there are no runtime crashes when
performing the metadata walk.
Note that the flag can be set directly in the target
definition, or inherited from the .gn default value set
through experimental_collect_validations_metadata.
- builder_record.h: Add a comment explaining why metadata
walks that include validations must be delayed.
- ninja_generated_file_target_writer.cc: If a target
has collect_validations_metadata enabled, do not write
its output file yet, instead set a boolean flag
to indicate the delay.
- ninja_target_writer.cc: Take an optional pointer to
a boolean flag to receive notification of delayed
writes for generated_file() targets. Pass the pointer
to NinjaGeneratedFileWriter properly.
- setup.cc: Add SetPostResolutionCallback() to register
a callback to invoke after all items have been resolved
in the graph (i.e. after the first scheduler.Run() call).
- setup_unittest.cc: Add new unit-test that verify that
metadata walks with validations do not crash and return
the correct values (or errors in case of missing deps).
- command_gen.cc: Record delayed generated_file() target
pointers during graph resolution, then use a post-resolution
callback to write all delayed generated_file() outputs
in parallel.
Benchmarking shows that:
- For a medium Fuchsia project (which currently relies on
validations metadata walks), using
`gn gen --experimental-collect-validations-metadata` is
about 15% slower with this patch applied (18.55s to
20.3s)
- For a default Android/arm64 Chromium build,
`gn gen --experimental-collect-validations-metadata`
there is no statical difference (around 3s each).
Bug: 566346002
Change-Id: I58ad0c3bcec71ceec80cf5a617ef00e86415734d
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27086
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Matt Stark <msta@google.com>
Commit-Queue: David Turner <digit@google.com>
Previously, ResolvedTargetData::ComputeHardDeps() and
NinjaTargetWriter::WriteHardDepsStampOrPhony() forwarded hard_deps from
both public_deps and private deps of a target T (unless T was a binary
target with public = []), because standard 'gn check' allows T's
public headers to #include headers from T's private deps.
However, when T sets check_includes_strict = true, 'gn check' enforces
that T's public headers cannot #include headers from T's private deps.
Therefore, targets depending on T cannot transitively reach generated
headers from T's private dependencies.
This change restricts ComputeHardDeps() and WriteHardDepsStampOrPhony()
to only traverse public_deps when check_includes_strict = true (for
non-hard targets), while still including hard_deps of private_deps in
T's own input dependencies in WriteInputDepsStampOrPhonyAndGetDep().
Suggested in:
https://chromium-review.googlesource.com/c/chromium/src/+/8497030/comment/9c2d1aca_83609b40/
Bug: 563098656
Change-Id: If9d5ef1e18475f0bcae1a65553256ff84543d871
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27100
Reviewed-by: Matt Stark <msta@google.com>
Reviewed-by: David Turner <digit@google.com>
Commit-Queue: Takuto Ikuta <tikuta@google.com>
The default is false, unless the .gn flag
experimental_collect_validations_metadata is set to true.
When this flag is true, metadata walks will visit validation
dependencies. This may result in runtime crashes due to the
data race described in the associated bug.
A future CL will ensure that walks that include validations
are always performed after all targets have been resolved
to fix the issue.
Bug: 566346002
Change-Id: I12a8d913aa3895de8ec991b3a491c45e41b0cbfa
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27085
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: David Turner <digit@google.com>
And associated --experimental-collect-validations-metadata
switch.
This is not connected to anything right now.
Bug: 566346002
Change-Id: Id374c42d29cca4d6cd62367596dc81f14fa5198e
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27084
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: David Turner <digit@google.com>
This simply moves NinjaGeneratedFileTargetWriter::GenerateFile()
to its own helper function in a new source file. The new
implementation simply returns false on error instead of
calling g_scheduler->FailWithError().
This is a preparation CL for a fix for the associated bug
that will delay the writing of generate_file() targets that
have `collect_validations_metadata = true`.
Bug: 566346002
Change-Id: I588a3cce0ba6252c65e650a04cb813d885f7295a
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27083
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: David Turner <digit@google.com>
Avoid dynamic string allocations + comparisons when comparing
labels during metadata collection.
Replace the GetUserVisibleValue(true) calls and associated
string comparisons with calls to a new Label::Matches() method
which can do the same comparison more quickly by performing
four pointer comparisons.
Change-Id: I51e1f81c49cc9d0a79f3f1310a0ba3113f52d406
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27082
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: David Turner <digit@google.com>
Define a struct holding the common state during recursive
metadata collection. The new MetadataWalker::Walk() method
makes recursive calls easier to see and use less parameters.
This doesn't change the logic, just moves the
implementation and perform a few renames:
+ Rename GetMetadata() first two parameters to data_keys
and walk_keys to match their GN argument names.
+ Rename next_walk_keys to next_walk_labels because
this list contains list of labels, not walk/data key
names.
Bug: 566346002
Change-Id: I33ea582da566d14d91ea6123c8541cf0906bb5a5
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27081
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: David Turner <digit@google.com>
Ensure that Scheduler::{is_failed_,has_been_shutdown_}
are properly guarded by a lock since they are accessed
from multiple threads concurrently.
In Scheduler::FailWithError(), ensure the PostTask()
call happens under the lock.
In particular, this can happen when several background worker
threads are posting tasks to the queue, and one of them calls
Scheduler::FailWithError() which will implicitly call PostQuit().
Change-Id: Ie0b70be0838b9fdcf4f41c3ae53bc5bd8e6d71d3
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27080
Commit-Queue: David Turner <digit@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
rustc does not invoke a linker when building an rlib, so non-Rust native
dependencies (object files of source_sets, static libraries and shared
libraries) are not used when compiling it. Since RUST_LIBRARY is not
final, these dependencies are already propagated as inherited libraries
to the final target that links the rlib.
However, NinjaRustBinaryTargetWriter added them to the rlib's rustc
command line (as -Clink-arg=...) and to its implicit ninja dependencies.
This made compiling an rlib wait for C/C++ code to be compiled, archived
and linked. For example, in a Chromium component build, compiling an
rlib depending on a C++ component waited for its shared library (and
those of its dependencies) to be linked, which put rlib compilation
after long chains of shared library links on the critical path.
Stop adding non-Rust link inputs to rlibs. Final targets still get them
through inherited libraries, as tested in NonRustDeps.
Bug: 563098656
Change-Id: Id5a80c39a44a4e347e8ad64505efe1e686171517
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27000
Reviewed-by: David Turner <digit@google.com>
Commit-Queue: David Turner <digit@google.com>
Reviewed-by: Matt Stark <msta@google.com>
Using `gn edit` with something like `remove testonly` results in a
non-fatal warning if the target does not have such attribute, and that
warning is collected in the `EditState` that `RunEditImpl` returns. This
run in UaF as every warning is an `Err` whose `Location` points into the
`InputFile` owned by the `BuildFile` it came from, however with the
that list being scoped to `RunEditImpl`, we end up with `RunEdit`
formatting those warnings using freed memory.
This CL fixes that by hoisting `build_files_` as a sibling field to
`edit_state_`.
Change-Id: I12acd0c02f3196c5f122642fadf6be2184769256
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26700
Reviewed-by: Matt Stark <msta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Write a rule for each target with at least two hard deps,
similar to what WritePublicInputsStampOrPhony() does for public_inputs:
build phony/foo/bar.harddeps: $
phony phony/foo/action phony/foo/dep.harddeps
It lists the target's direct hard deps. This mirrors
ResolvedTargetData::ComputeHardDeps(), so it depends on the same set
of hard deps, but its size is proportional to the number of direct deps.
A target's input deps then just refer to its own .harddeps rule.
Results for a Chromium build (mac, arm64, is_debug=false):
before after
size of all loaded .ninja files 1025 MB 427 MB
gn gen (hyperfine, 6 runs) 2.065s ± .048 1.947s ± .050
user / sys 12.0s / 4.3s 10.1s / 4.5s
Chromium no-op builds of base_unittests (target already built, hyperfine, M-series Mac):
before after
siso v1.6.0 (autoninja), -offline 735.5 ms ± 13.7 648.3 ms ± 22.8
user / sys 3.7s / 1.9s 2.3s / 2.2s
depot_tools ninja (1.12.1) 4.37s ± .02 2.25s ± .02
ninja v1.13.2 2.755s ± .009 1.532s ± .007
ninja with some local changes to tweak perf 535.8 ms ± 2.7 408.4 ms ± 8.1
(`siso -n` and `ninja -n` are within 2% of the no-op times.)
No intended behavior change.
Bug: chromium:40278737
Change-Id: I163e6bea334c2e626bf7c96cd8d942848a6766bf
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26741
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Nico Weber <thakis@google.com>
Commit-Queue: Nico Weber <thakis@chromium.org>
Suppose you have foo_linux.cc including bar.h, and:
```
source_set("foo") {
sources = [ "foo.cc" ]
if (is_linux) {
sources += [ "foo_linux.cc" ]
}
}
```
Previously, we would suggest `gn edit "add deps //:bar"` //:foo`.
Now, we no longer output the gn edit suggestion, and instead suggest
doing so inside an is_linux conditional.
Change-Id: Iac7fb49959fb6437b5ffd2595614bf526a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26680
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
The size of the generated ninja files is quadratic: NinjaCBinaryTargetWriter writes an order-only dep for every non-linkable
dep of a target. `source_set("a")` depending on `source_set("b")`
depending on `source_set("c")` produced:
build phony/c.linkdeps: phony c.o
build phony/b.linkdeps: phony b.o || phony/c.linkdeps
build phony/a.linkdeps: phony a.o || phony/b.linkdeps phony/c.linkdeps
a.linkdeps listing c.linkdeps is not needed.
Instead, only write order-only deps for:
1. Direct deps (looking through groups, which have no build statement).
2. Libraries inherited through direct deps whose build statement doesn't
list them (e.g. Rust libraries, or final targets that forward shared
libraries).
3. Frameworks (create_bundle targets). ClassifyDependency() doesn't
treat them as non-linkable deps of non-final targets.
https://issues.chromium.org/issues/40278737#comment7 compares ninja
files before and after this patch and checks that they are equivalent.
Results for a Chromium build (mac, arm64, is_debug=false):
before after
size of all loaded .ninja files 2634 MB 1009 MB
.linkdeps phony edges 1442 MB 15 MB
`||` on alink edges 181 MB 5 MB
`||` on link edges 21 MB 0.3 MB
gn gen (hyperfine, 6 runs) 2.917s ± .047 2.160s ± .052
user / sys 18.5s / 7.2s 12.3s / 4.7s
Chromium no-op builds of base_unittests (target already built, hyperfine,
M-series Mac), for siso and various ninja versions:
before after
siso v1.6.0 (autoninja), -offline 1.058s ± .031 735.5 ms ± 13.7
user / sys 7.0s / 2.0s 3.7s / 1.9s
depot_tools ninja (1.12.1) 10.09s ± .10 4.37s ± .02
ninja v1.13.2 6.388s ± .049 2.755s ± .009
ninja with some local changes to tweak perf 950.9 ms ± 5.1 535.8 ms ± 2.7
(`siso -n` and `ninja -n` are within 2% of the no-op times.)
No intended behavior change.
Bug: chromium:40278737
Change-Id: I517db1c641001e404d3e02ab0e653c1133076d7f
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26740
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Nico Weber <thakis@google.com>
don't resolve to a single target.
This made a lot of sense when you had :foo1 and :foo2 both contain foo.cc in their sources,
and you ran `gn suggest foo.cc=bar.h`, but makes less sense when gn
check complains that foo1 doesn't depend on bar and foo2 also doesn't
depend on bar.
Change-Id: I1111e7c1b623015047721cf35e23420d6a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26660
Commit-Queue: Matt Stark <msta@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
When generating rust-project.json via `gn gen --export-rust-project`,
RustProjectWriter previously emitted only
`dep->rust_values().crate_name()` for crate dependencies in `"deps"`,
ignoring `target->rust_values().aliased_deps()`.
By contrast, NinjaRustBinaryTargetWriter checks `aliased_deps()` when
passing `--extern <alias>=<rlib>` flags to `rustc`. When a target uses
`aliased_deps` (such as renamed third-party Cargo crates),
`rust-analyzer` and Kythe indexers consuming `rust-project.json` could
not resolve imports using the aliased name.
This change updates `AddTarget` in `rust_project_writer.cc` to check
`target->rust_values().aliased_deps()` and emit the aliased dependency
name alongside `crate_name()`.
Bug:513301934
CONV=1d75e1e8-412c-4982-8ca8-e70282d906f4
Change-Id: I9f90915d1c252aab24c3dcdc7552b115b6ef9779
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26620
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Marc Jin <jmarc@google.com>
A header can belong to several targets. Diagnosing each candidate
during the search can build unrestricted reachability tables and
errors that are discarded when a later candidate permits the include.
Separate acceptance checks from diagnostics. First search for a target
that permits the include, checking visibility, permitted dependency
chains, include exceptions and strict checking rules. Only enter the
diagnostic pass if the include is rejected.
Chromium `gn gen --check`, medians of five round-robin rounds:
Mac (M4 Max): 2.70 s -> 2.62 s
GCE c4d-384: 4.22 s -> 4.08 s
Intel 13900KS: 2.55 s -> 2.33 s
CPU time (c4d-standard-384):
User: 31.6 s -> 28.6 s (-9%)
Sys: 33.9 s -> 34.0 s (0%)
Heap stats:
Allocs (count): 53.78 M -> 53.51 M (-1%)
Heap (total): 19.78 GB -> 18.10 GB (-8%)
Heap (peak): 3.22 GB -> 2.30 GB (-29%)
Change-Id: I8c0118801072da1d37b7da291e5c047f9abfcd29
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26343
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Philipp Wollermann <philwo@google.com>
Reviewed-by: Matt Stark <msta@google.com>
Precompute only the dependency reachability needed to permit includes.
Leave the unrestricted walk, which also follows private dependencies
transitively, to the existing lazy lookup when no permitted path is
found. This avoids building large diagnostic tables for every checked
target.
Use a shared lock for reachability cache hits, taking the exclusive
lock only on a miss.
Chromium `gn gen --check`, medians of five round-robin rounds:
Mac (M4 Max): 3.13 s -> 2.70 s
GCE c4d-384: 5.15 s -> 4.22 s
Intel 13900KS: 3.30 s -> 2.55 s
CPU time (c4d-standard-384):
User: 38.5 s -> 31.6 s (-18%)
Sys: 50.2 s -> 33.9 s (-32%)
Heap stats:
Allocs (count): 54.24 M -> 53.78 M (-1%)
Heap (total): 24.92 GB -> 19.78 GB (-21%)
Heap (peak): 5.46 GB -> 3.22 GB (-41%)
Change-Id: Idbd45609b60d465f78b240db93a33695af34c131
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26342
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Philipp Wollermann <philwo@google.com>
Reviewed-by: Matt Stark <msta@google.com>
Post reachability precomputation in groups of 32 targets and file
checking in groups of 64 files. This reduces queue operations and
completion counter updates. Collect violations within each group and
merge them under one lock.
Share the task posting and completion wait between both phases.
Chromium `gn gen --check`, medians of five round-robin rounds:
Mac (M4 Max): 3.07 s -> 3.13 s (within noise)
GCE c4d-384: 5.80 s -> 5.15 s
Intel 13900KS: 3.24 s -> 3.30 s (within noise)
CPU time (c4d-standard-384):
User: 39.6 s -> 38.5 s (-3%)
Sys: 45.4 s -> 50.2 s (+11%)
Heap stats:
Allocs (count): 54.24 M -> 54.24 M (0%)
Heap (total): 24.93 GB -> 24.92 GB (0%)
Heap (peak): 5.47 GB -> 5.46 GB (0%)
Change-Id: I8b884c5dd5c1bd58a40aaf151073ce9058326501
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26341
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Matt Stark <msta@google.com>
Commit-Queue: Philipp Wollermann <philwo@google.com>
Build the list of files to check by filtering the complete file map.
This avoids rebuilding a second map and repeating the work to merge
each target's source, public and generated file metadata.
Derive the targets for reachability precomputation from the selected
files, so targets without eligible files do not get caches. Add a test
for this selection.
Chromium `gn gen --check`, medians of five round-robin rounds:
Mac (M4 Max): 3.16 s -> 3.07 s
GCE c4d-384: 5.96 s -> 5.80 s
Intel 13900KS: 3.42 s -> 3.24 s
CPU time (c4d-standard-384):
User: 40.0 s -> 39.6 s (-1%)
Sys: 53.0 s -> 45.4 s (-14%)
Heap stats:
Allocs (count): 55.16 M -> 54.24 M (-2%)
Heap (total): 25.58 GB -> 24.93 GB (-3%)
Heap (peak): 5.74 GB -> 5.47 GB (-5%)
Change-Id: Iaa4971421bf860d9453dc4ebfa5fd65452a1980c
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26340
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Philipp Wollermann <philwo@google.com>
Reviewed-by: Matt Stark <msta@google.com>
Previously, BuilderRecord::waiting_map_ was a base::flat_map. For
heavily depended-upon targets (such as core libraries like //base:base
and popular configs) that have thousands of dependents, inserting a new
dependent in waiting_map_ caused a binary search followed by an O(N)
memmove of all subsequent elements to keep the flat_map sorted.
Once StringAtomSet global lock contention is eliminated, worker threads
parse and submit targets to the Builder at a much higher rate, making
these O(N) memmove insertions a major bottleneck on the main thread.
However, waiting_map_ only requires point lookup by pointer and full
traversals to notify dependents of state changes; it never requires
sorted order (and functions like GetSortedUnresolvedDeps already sort
their output explicitly).
Switching to std::unordered_map eliminates the array shifts on insertion.
Benchmark results (`gn gen out/Default` on Chromium, 5 runs with hyperfine,
official release build configuration with Fuchsia Clang + libc++ +
static libjemalloc.a + LTO):
- Stacked on StringAtomSet sharding (CL 26540):
2.258s ± 0.035s -> 2.060s ± 0.064s (-198ms / -8.8%, min 2.011s)
- Compared to origin/main:
2.755s ± 0.029s -> 2.060s ± 0.064s (-695ms / 1.34x faster)
Output ninja files remain bit-for-bit identical across all 14,639 files.
TAG=agy
CONV=4cb5d931-60a8-4f44-8632-3e8d9fb10a8d
Change-Id: I3174ee798a5f6d0c58736fc05ad4ed2684b1d37c
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26560
Reviewed-by: Matt Stark <msta@google.com>
Commit-Queue: Takuto Ikuta <tikuta@google.com>
Previously, StringAtomSet used a single global std::mutex to protect the
global KeySet and Slab storage. On high-core-count machines, worker
threads frequently miss their thread-local cache for new strings across
thousands of files, causing intense lock contention on this single
mutex.
Profiling Chromium's //out/Default build generation with official GN
release build settings (Fuchsia Clang + libc++ + static libjemalloc.a +
LTO) showed that kernel futex waits on StringAtomSet::find() accounted
for over 70% of the total System CPU time.
This change shards StringAtomSet into 256 independent, cache-aligned
(alignas(64)) partitions indexed by the string hash's upper bits. In
addition, both StringAtomSet and ThreadLocalCache pre-insert
kEmptyString in their constructors so that empty strings hit directly
without slab allocation or extra branches in the lookup hot path.
Benchmark results (`gn gen out/Default` on Chromium, 5 runs with
hyperfine, official release build configuration):
- Wall-clock time: 2.755s ± 0.029s -> 2.262s ± 0.038s (-493ms / 1.22x faster)
- System CPU time: 14.079s -> 4.030s (-10.05s / -71.4% reduction!)
- User CPU time: 23.740s -> 22.889s (-0.85s)
Output ninja files remain bit-for-bit identical across all 14,639 files.
TAG=agy
CONV=4cb5d931-60a8-4f44-8632-3e8d9fb10a8d
Change-Id: I2d09d9f25c6c86482ef420282a41606308b8d8d4
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26540
Commit-Queue: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Matt Stark <msta@google.com>
Follow-up to https://gn-review.googlesource.com/c/gn/+/26480.
As pointed out by Matt Stark (msta@), having a lazy
'info_.emplace(this)' fallback inside Target::info() could theoretically
allow concurrent mutation if info() were ever called on an unresolved
target in a multi-threaded context.
In production, Target::OnResolvedWithoutChecks() is always called before
any multi-threaded queries occur. Across the entire unit test suite (845
tests), only a single test (NinjaCBinaryTargetWriterTest.
DupeObjFileError) invoked NinjaCBinaryTargetWriter without calling
OnResolved() first.
By adding 'ASSERT_TRUE(target.OnResolved(&err))' to DupeObjFileError, we
can replace the lazy initialization in Target::info() with
'DCHECK(info_.has_value())', making info() strictly read-only and
branchless in release builds.
Note: 'info_' itself remains 'mutable' because ResolvedTargetData
queries receive 'const Target*' and lazily compute/populate the cache
fields inside TargetInfo (e.g., lib_dirs, inherited_libs). Keeping
'mutable' on 'info_' as a whole avoids scattering 'mutable' across ~20
individual fields inside TargetInfo and modifying all helper signatures.
TAG=agy
CONV=4cb5d931-60a8-4f44-8632-3e8d9fb10a8d
Change-Id: I6a697e0172473d38b80ca04ea0985c6dab8e3d44
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26500
Commit-Queue: Takuto Ikuta <tikuta@google.com>
Reviewed-by: Matt Stark <msta@google.com>
Profiling 'gn gen' on Chromium 'out/Default' (36k targets, 64 cores)
revealed that ResolvedTargetData::GetTargetInfo was a major bottleneck:
- std::__Cr::__shared_mutex_base::lock_shared() consumed 9.01% cycles.
- Hash computation (CityHash/hash_memory) consumed 14.35% cycles.
Together, about 23% of total CPU cycles were spent on every query
acquiring shared_locks and probing shard hash maps, even though targets
are queried repeatedly and their pointers are immutable after
resolution.
As suggested by Matt Stark (msta@), we can simplify this significantly:
1. Move TargetInfo directly into Target as 'std::optional<TargetInfo>'.
2. Initialize 'info_' when Target::OnResolvedWithoutChecks() is called
(with a fallback lazy initialization in info() for unit tests that
skip OnResolved).
3. Make ResolvedTargetData completely stateless. GetTargetInfo() simply
returns '&target->info()' inline with zero locks, zero hashing, and
zero dynamic heap allocations for TargetInfo map entries.
4. Because Target owns its TargetInfo directly, the lifetime of
TargetInfo perfectly matches Target, eliminating any possibility of
use-after-free or address-reuse (ABA) bugs when Targets or
ResolvedTargetData instances are destroyed in unit tests.
Performance impact on Chromium 'out/Default' (vs origin/main):
- Wall-clock time: 3.281s -> 3.173s (-108ms, -3.3%)
- User CPU time: 30.87s -> 27.32s (-3.55s, -11.5%)
- CPU cycles: 102.91G -> 89.86G (-13.05G cycles, -12.7%)
- Instructions: 110.14G -> 102.72G (-7.42G instrs, -6.7%)
All 845 unit tests passed. Output ninja files are bit-for-bit identical.
TAG=agy
CONV=4cb5d931-60a8-4f44-8632-3e8d9fb10a8d
Change-Id: Ib23f999522044d5b4b46128491d54ef9061ef4b5
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26480
Reviewed-by: Matt Stark <msta@google.com>
Commit-Queue: Takuto Ikuta <tikuta@google.com>
Configure jemalloc with --with-malloc-conf=thp:always to enable
Transparent Huge Pages (THP) by default on Linux.
When running large GN generation tasks (such as Chromium's gn gen),
jemalloc allocates extensive virtual memory ranges. Without THP enabled
by default, allocations rely heavily on 4KB pages, leading to
significant kernel overhead in page fault handling and page table walks
(minor page faults: ~467k).
By enabling thp:always, jemalloc proactively uses 2MB huge pages via
madvise(MADV_HUGEPAGE), reducing minor page faults by 54% and improving
wall clock time by ~218ms (~6.7%).
Benchmark (`hyperfine --warmup 2 --runs 10` on Chromium `gn gen`):
- jemalloc (default): 3.236 s ± 0.028 s
- jemalloc (thp:always): 3.018 s ± 0.052 s (-218 ms, 1.07x faster)
perf stat comparison:
- Minor faults: 467,490 -> 215,229 (-54.0%)
- CPU cycles: 104.9G -> 98.2G (-6.7B cycles)
- User CPU time: 26.54s -> 23.92s (-2.62s)
- Sys CPU time: 17.51s -> 15.62s (-1.89s)
TAG=agy
CONV=4cb5d931-60a8-4f44-8632-3e8d9fb10a8d
Change-Id: I134509479c0cf74c397cea8c8227d911e2734963
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26460
Commit-Queue: David Turner <digit@google.com>
Reviewed-by: David Turner <digit@google.com>
Resolving each include against each include directory constructs a
SourceFile, which interns the candidate path even when lookup fails.
This adds allocations and contention on the shared interning lock.
Use a hash map keyed by path string views. Normalize candidate paths
in a reusable buffer, look them up directly, and return the SourceFile
stored in the matching entry.
Chromium `gn gen --check`, medians of five round-robin rounds:
Mac (M4 Max): 3.91 s -> 3.16 s
GCE c4d-384: 7.71 s -> 5.96 s
Intel 13900KS: 4.11 s -> 3.42 s
CPU time (c4d-standard-384):
User: 43.8 s -> 40.0 s (-9%)
Sys: 121.1 s -> 53.0 s (-56%)
Heap stats:
Allocs (count): 64.84 M -> 55.16 M (-15%)
Heap (total): 26.27 GB -> 25.58 GB (-3%)
Heap (peak): 5.95 GB -> 5.74 GB (-4%)
Change-Id: Iefb962d72efcba69361bccce430e41d10a63c2a9
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26320
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Philipp Wollermann <philwo@google.com>
It is more preferred to split into a source set if you have public
depending on a private header.
Conveniently, this also means that `gn check --fix` will never succeed
and then break if you run it immediately after (which it would
previously do because the header that was made public may require
something to be moved from deps to public_deps)
Change-Id: I762a529c279232c6d7edb94bf99eecbc6a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26400
Commit-Queue: Matt Stark <msta@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
When running `gn check --fix`, if you added a #include to both the .cc
and the .h file, you will get the following suggestions:
* "add deps //dep"
* "add public_deps //dep"
This ensures that it will only get added to public_deps.
Additionally, change gn suggest to recommend adding to public_deps
instead of moving from deps to public_deps.
Change-Id: I59074724bb5ed61a3499f1a81d2f3b356a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26380
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
Record the source target on HeaderChecker::Violation so that CheckPublicHeaders
passes the actual toolchain of the violating target to OutputSuggestions,
preventing incorrect cross-toolchain suggestions.
Change-Id: I7625f30ced367c2f8680e673fc5a5d0b6a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26360
Commit-Queue: Matt Stark <msta@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Together with the previous optimizations, this changes the time
complexity of suggestions be no longer proportional to the number of targets in the build graph.
To benchmark this, I changed the allowlist for `check_includes_strict`
to //*, then ran `gn check`.
Before: Outputs a few errors per second. Left it running for a few
minutes and I had no way to tell how long it was going to take, but
probably hours.
After: Outputs 295,000 errors in 22 seconds.
Change-Id: I4658aa7f612400eec44870c7dc962f176a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26242
Commit-Queue: Matt Stark <msta@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
This is part of a larger process to improve the performance of
suggestions, but does not have change the time complexity until gn-review.googlesource.com/c/gn/+/26242 is submitted (benchmarks in that commit)
Change-Id: Ic0bd366f7b2639f16a9e633dde527cec6a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26281
Commit-Queue: Matt Stark <msta@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
This is part of a larger process to improve the performance of
suggestions, but does not have change the time complexity until gn-review.googlesource.com/c/gn/+/26242 is submitted (benchmarks in that commit)
Change-Id: I6e0067ca86f1e1afc6c8a828004011fd6a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26260
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
This is part of a larger process to improve the performance of
suggestions, but does not have change the time complexity until gn-review.googlesource.com/c/gn/+/26242 is submitted (benchmarks in that commit)
Change-Id: Ia47be0e501d26be321ad2fdc8681750b6a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26240
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
`gn check --fix` currently outputs errors when it tries to apply the
same move twice. This fixes move to be a no-op when it's already
applied, similar to other commands.
Change-Id: Id0b8182d3fc8e9f9d6d308951f387f766a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26220
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
This ensures that when you run:
```
CXX=PATH_TO_CXX build/gen.py ...
ninja -C out
```
PATH_TO_CXX is actually used when building
Change-Id: Ie9b7fb149bd37181e7e1ba76024a9a806a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26180
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Matt Stark <msta@google.com>
aphrody-dev
Ryosuke Miyake