diff --git a/.github/workflows/benchmark_backfill.yml b/.github/workflows/benchmark_backfill.yml index 4a92586353c..05f73afd42c 100644 --- a/.github/workflows/benchmark_backfill.yml +++ b/.github/workflows/benchmark_backfill.yml @@ -95,7 +95,7 @@ jobs: # Build the current accounting adapter before the driver freezes the batch. bazel build //tools:process_resources_native --config=clang_release \ --disk_cache="${HOME}/.cache/bazel-disk" - launcher="$(bazel cquery //tools:process_resources_native --config=clang_release \ + launcher="$(bazel cquery 'config(//tools:process_resources_native, target)' --config=clang_release \ --output=starlark --starlark:expr='target.files_to_run.executable.path')" cp "${launcher}" "${RUNNER_TEMP}/benchmark-resource-launcher" fixture_parent="${RUNNER_TEMP}" diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml index 56785a064fb..328a9348abb 100644 --- a/.github/workflows/benchmarks.yml +++ b/.github/workflows/benchmarks.yml @@ -59,11 +59,12 @@ jobs: head_sha="$(git rev-parse HEAD)" base_sha="$(git rev-parse HEAD^1)" git worktree add --detach "${RUNNER_TEMP}/benchmark-base" "${base_sha}" - bazel build //xff/cli:xff //xff/engine:read_benchmark //tools:benchmark_resources //xff/engine:expression_benchmark //xff/engine:expression_run_benchmark --config=clang_release \ + # Python runfiles may build a transitioned helper, not the target copied below. + bazel build //xff/cli:xff //xff/engine:read_benchmark //tools:benchmark_resources //tools:process_resources_native //xff/engine:expression_benchmark //xff/engine:expression_run_benchmark --config=clang_release \ --disk_cache="${HOME}/.cache/bazel-disk" cp bazel-bin/xff/cli/xff "${RUNNER_TEMP}/benchmark-head" cp bazel-bin/xff/engine/read_benchmark "${RUNNER_TEMP}/benchmark-head-reads" - launcher="$(bazel cquery //tools:process_resources_native --config=clang_release \ + launcher="$(bazel cquery 'config(//tools:process_resources_native, target)' --config=clang_release \ --output=starlark --starlark:expr='target.files_to_run.executable.path')" cp "${launcher}" "${RUNNER_TEMP}/benchmark-resource-launcher" driver="$(pwd)/$(bazel cquery //tools:benchmark_resources --config=clang_release \ diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 43352a9192a..ff90fab37f4 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -752,7 +752,7 @@ jobs: run: | bazel build //xff/cli:xff //tools:process_resources_native //xff/engine:expression_benchmark //xff/engine:expression_run_benchmark --config=clang_release --disk_cache="${HOME}/.cache/bazel-disk" cp bazel-bin/xff/cli/xff "${RUNNER_TEMP}/benchmark-head" - launcher="$(bazel cquery //tools:process_resources_native --config=clang_release \ + launcher="$(bazel cquery 'config(//tools:process_resources_native, target)' --config=clang_release \ --output=starlark --starlark:expr='target.files_to_run.executable.path')" cp "${launcher}" "${RUNNER_TEMP}/benchmark-resource-launcher" tar -cf benchmark-build.tar -C "${RUNNER_TEMP}" benchmark-head benchmark-resource-launcher diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6af137e5ca1..a062db3a09e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -100,6 +100,12 @@ the local hook reports a skip; the dedicated CI job supplies both and enforces t source missing from an existing database fails with a refresh instruction instead of silently omitting that file. +Expected-source queries fail closed: a Bazel query error is reported with its +package pattern and stops indexing before the database can claim completeness. +A successful headers-only query may be empty. The source collector publishes no +partial expected-source list when any query fails; `//tools:collect_compile_sources_test` +checks both cases without invoking a real build. + Check selected files, including previously committed files that failed CI: ```sh diff --git a/TODO.md b/TODO.md index 71705ad7820..be210876e07 100644 --- a/TODO.md +++ b/TODO.md @@ -73,6 +73,59 @@ Detailed scope, data-structure audit, benchmark matrix and decision gates: - [ ] EP-S02: Revisit experimental MBO interning for dynamic names only when a concrete consumer and measurements justify a dependency update; prefer existing strong IDs and indexed execution slots. +## Stage-specific concurrency and execution scheduling + +New insight from the PR #997 review, recorded without changing that PR's bounded-frontier +scope or declaring new flags implemented. See +[the parallel design follow-up](docs/design-parallel.md#follow-up-insight-stage-specific-concurrency-controls). + +- [ ] JC01: Design a `--jobs-*` family separating directory I/O, matching/comparison and external + execution. Decide eager-metadata ownership, caller-inclusive counts, role versus aggregate + ceilings, `--jobs` fallback/override precedence, `all`, defaults and INI/repeated-option behavior. + Preserve existing commands without new controls; illustrative spellings are not stable APIs. + Use an `--unstable=NAME` gate if implementation begins before spelling is settled, and add + registry/help/man/reference documentation and parsing/configuration tests in the same change. +- [ ] JC02: Separate one-at-a-time queued command execution from strict synchronous expression + evaluation. Permit independent directory discovery alongside a single persistent execution + runner; retain child-exit-status gating, short-circuiting, chained actions and conditional + output when synchronous semantics are requested. Define explicitly parallel execution without + silently adopting success-on-launch semantics or changing `-exec ... +` batching. +- [ ] JC03: Audit traversal/effect dependencies before enabling a fully concurrent pipeline. + Preserve prune/quit/depth, captures, reductions, mutations and ordered output; distinguish + speculative reads from visits/actions and never assume arbitrary external commands are pure. + Define cancellation/draining and failure propagation with exactly-once effect controls. +- [ ] JC04: Compare role-aware shared execution with coordinated persistent stage pools. Reuse threads, + reserve useful CPU/execution capacity despite blocked directory I/O, forbid nested pool waits, + and bound queued work, retained results and execution arguments with backpressure. Track threads, + participants and outstanding children separately; concurrent runners/captures must reap only + their own child identities. Verify ownership and concurrency limits with sanitizer controls. +- [ ] JC05: Qualify filesystem/hardware-dependent directory concurrency rather than use CPU count + or a universal small-reader cap as a proxy. Test few-reader local laptop/PC cases and tens-of-reader + network cases, including the user's roughly three-versus-50 concurrency examples as hypotheses, + not measured XFF defaults. Allow explicit I/O limits beyond detected cores; evaluate adaptive + admission from remaining work, observed latency and queue pressure with bounded probing. +- [ ] JC06: Measure the mixed-stage configurations on native Linux and macOS with local, slow and + network storage. Preserve raw output/provenance and compare startup, reused-worker scheduling, + useful task size, throughput, first output, memory and actual thread/child counts. Include serial + command/concurrent-discovery, strict dependent expressions and parallel command controls. + Do not accept a default or performance claim from hosted correctness checks alone. +- [ ] JC07: Separate directory-admission tuning from worker-budget tuning. PR #991's experimental + 64-sibling gate is not an optimum across storage types. Design explicit serial operation and + minimum independent-work/cost controls in the jobs family, preserving semantic/safety barriers + and command resource bounds. Distinguish cold start from warm dispatch, and qualify settings + on small slow frontiers as well as broad memory-backed workloads before choosing defaults. + Keep spellings unsettled until designed; follow JC01's unstable gate and self-documentation. +- [ ] JC08: Add opt-in, super-low-overhead scheduling analysis for admission tuning. Select diagnostics + once at command setup; keep disabled inner loops free of diagnostic checks, timers, counters, + atomics and allocations. Use bounded per-role/worker aggregates from existing observations: + fan-out, useful work, admission/refusal reasons, cold/warm worker use, queue pressure and caller + participation. Add only coarse or sampled timing where needed; never add filesystem probes, + force worker creation, reorder work or retain unbounded per-path traces for analysis. + Report actual versus estimated quantities separately, preserve normal output, and prefer an + existing explain/summary interface where it fits. Prove correctness and inspect disabled code + paths; measure both disabled/off-baseline cost and enabled/off overhead on native Linux/macOS + and representative storage. Super-low overhead is an acceptance target, not a zero-effect claim. + ## Pages deployment size - [x] Redirect byte-identical stable coverage HTML to preserved run archives in all publishers; diff --git a/compile_commands-update.sh b/compile_commands-update.sh index 381e7d07024..c7dccdd260b 100755 --- a/compile_commands-update.sh +++ b/compile_commands-update.sh @@ -168,10 +168,14 @@ bazel run --config=clang-tidy //:refresh_compile_commands -- --config=clang-tidy # `extra_modules/archive/...`). EXPECTED_SOURCES="$(mktemp)" trap 'rm -f "${EXPECTED_SOURCES}"' EXIT -for pattern in "//xff/..." $(python3 tools/extras.py --wildcards) "@xff_extras_api//..."; do - bazel query "filter(\"\\.cc$\", kind(\"source file\", deps(kind(\"cc_.* rule\", ${pattern}))))" 2>/dev/null \ - | grep -E '^@@(//|xff_)' >>"${EXPECTED_SOURCES}" || true +declare -a SOURCE_QUERY_PATTERNS=() +EXTRA_SOURCE_QUERY_PATTERNS="$(python3 tools/extras.py --wildcards)" \ + || die "loading expected-source package patterns failed" +for pattern in "//xff/..." ${EXTRA_SOURCE_QUERY_PATTERNS} "@xff_extras_api//..."; do + SOURCE_QUERY_PATTERNS+=("${pattern}") done +tools/collect_compile_sources.sh "${SOURCE_QUERY_PATTERNS[@]}" >"${EXPECTED_SOURCES}" \ + || die "collecting expected compile sources failed" python3 tools/fix_compile_commands.py compile_commands.json MODULE.bazel bazelmod/extras.MODULE.bazel \ --system="$(uname -s)" \ diff --git a/docs/design-parallel.md b/docs/design-parallel.md index 141e1a55f37..f3d90af2668 100644 --- a/docs/design-parallel.md +++ b/docs/design-parallel.md @@ -170,6 +170,101 @@ design-config.md): - `-j N` overrides the worker count in every style; `--sort=...` overrides the default ordering in every style. +## Follow-up insight: stage-specific concurrency controls + +Status: design insight and open work, not implemented flags or a change to the +bounded-frontier experiment in PR #997. Track the work under +[Stage-specific concurrency and execution scheduling](../TODO.md#stage-specific-concurrency-and-execution-scheduling). + +One worker number cannot express all useful combinations. A user may want concurrent +directory discovery while running external commands one at a time, or many independent +commands while keeping directory I/O deliberately narrow. Directory reads, eager metadata, +content evaluation, file comparison and external processes have different costs and +dependencies; scheduling should distinguish these roles rather than conflate their limits. + +Directory concurrency is also a filesystem/hardware policy, not simply the detected CPU +count. High-latency network storage can benefit from tens of concurrent directory operations; +the motivating user report described roughly 50 busy cores in a datacenter. A laptop or PC +may reach diminishing returns with only a few readers, with roughly three suggested as a +starting point for measurement, not a universal limit. Neither example supplies a default +or a measured XFF acceptance result. Explicit I/O concurrency must not be silently clamped +to the logical CPU count, and an automatic policy needs native evidence across storage types. + +Explore a `--jobs-*` family for separate stage limits, retaining `--jobs` as the convenient +existing control. Illustrative spellings are `--jobs-walk`, `--jobs-match`, `--jobs-compare` +and `--jobs-exec`; these are not accepted CLI options. Decide where eager metadata belongs, +whether `--jobs` becomes a fallback or remains an aggregate ceiling when overrides are +present, what counts as a participant, and how `all`, defaults, repeated options and INI +configuration interact. Preserve existing behavior when no new control is requested. +Do not ship provisional spellings as stable flags: unsettled spellings require the +repository's `--unstable=NAME` gate and complete registry-driven documentation when implemented. + +Worker limits and admission thresholds need separate tuning. PR #991's fixed 64-sibling +gate is an experimental policy, not a filesystem-independent optimum: a small slow frontier +may justify concurrency while a larger cached frontier does not. Explore jobs-family controls +for explicit serial directory operation and minimum independent work or estimated cost, +accounting separately for cold worker creation and warm dispatch. Overrides must preserve +effect dependencies, bounded queue/storage ownership and command-wide resource limits. +Do not settle a CLI spelling or a default from the current memory-backed Linux pilot alone. + +Tuning needs scheduling evidence without becoming a separate intrusive workload. The target +is opt-in, super-low-overhead analysis, selected once during command setup so disabled runs +keep their normal inner loops: no diagnostic flag checks, timers, counters, extra atomics +or diagnostic allocations per entry. Prefer a separate instrumented execution path or another +design that demonstrably preserves that property; a disabled boolean tested on every entry +does not meet it. No zero-cost claim follows merely from making the option default-off. + +When enabled, collect bounded worker-local or per-role aggregates from information already +available: directory fan-out, remaining useful work, admission/refusal reasons, cold versus +reused workers, queue depth/pressure and caller participation. Distinguish measured quantities +from cost-model predictions. Coarse phase boundaries or explicitly sampled timing can explain +waiting and utilization, but per-file timers and shared hot counters need measured justification. +Do not add filesystem reads/metadata probes, start workers solely for diagnostics, reorder +execution, or retain an unbounded list of paths or events. Preserve ordinary stdout and its +ordering; define a separate diagnostic destination and bounded output contract. Consider +extending an existing explain/summary interface rather than inventing a namespace of flags. + +Qualify analysis as a feature in its own right: output identity, effects, concurrency and +cancellation must match with diagnostics off/on. Inspect disabled compiled hot paths and +compare disabled execution against an uninstrumented baseline; separately measure enabled/off +overhead with retained raw samples on native Linux and macOS and local, slow and network +storage. Declare the sampling and overhead budget before acceptance, and report any remaining +perturbation. Super-low overhead is sufficient for tuning; an unverified "non-intrusive" label +or instrumented timings pooled into clean acceptance measurements are not. + +Serial external execution and synchronous expression evaluation are different contracts: + +- A bounded ordered execution queue with one persistent runner can serialize external + commands while independent directory discovery continues. More runners or outstanding + children can serve explicitly parallel execution. Queue depth, retained bytes, running + child processes and runner threads need distinct accounting and backpressure. +- Strict synchronous `-exec ... ;` evaluation must wait for the child's exit status before + evaluating a dependent expression continuation. A serial queue alone does not restore + that truth value. Preserve `-a`/`-o` short-circuiting, chained actions and conditional output; + never silently substitute the existing parallel mode's success-on-launch semantics. +- Prune, quit, post-order, stateful reductions, captures and filesystem-changing actions + introduce barriers. Apply each required decision before releasing dependent traversal or + effects. Safe directory read-ahead need not disable all concurrency, but speculative + reads must not become visits or actions in a pruned subtree. Arbitrary child commands + cannot be assumed pure or independent of later filesystem observations. + +Full multithreading should mean an explicitly bounded pipeline of independent work, not +unbounded recursive walks or a new thread for each entry. Evaluate one command-owned executor +with role-aware admission versus coordinated persistent stage pools. In either design, record +aggregate resource bounds, retain worker-private evaluation state, keep ordered publication and +borrowed-state lifetimes safe, and prevent blocked I/O from consuming every execution/CPU slot. +Workers must not submit work to their own bounded pool and then wait for it. Concurrent process +management must reap its own child identities rather than let a generic reaper consume children +owned by captures or another runner. + +Qualification must cover the useful mixed cases: narrow local discovery with parallel CPU work, +high-concurrency network discovery, serial commands with concurrent discovery, strict +exit-status-dependent execution, and explicitly parallel commands. Verify actual thread/child +counts, ordering, exactly-once effects, cancellation, failure propagation and queue/storage +limits. Measure cold startup, steady-state reuse, task size, throughput, first output and peak +memory on native Linux and macOS, including slow/network storage. More threads or hosted CI +success alone cannot establish a performance improvement or a portable default. + ## Concurrency correctness - **`-prune`** - the coordinator evaluates the entry and suppresses descent when diff --git a/tools/BUILD.bazel b/tools/BUILD.bazel index 3dfdc58290b..25ce971a70e 100644 --- a/tools/BUILD.bazel +++ b/tools/BUILD.bazel @@ -53,6 +53,15 @@ bashtest( ], ) +bashtest( + name = "collect_compile_sources_test", + srcs = ["collect_compile_sources_test.sh"], + data = [ + "collect_compile_sources.sh", + "collect_compile_sources_fake.sh", + ], +) + # The MSan RUNTIME suppression file, exposed so xff/cc.bzl's cc_binary / cc_test wrappers can put it # in each executable's `data` (runfiles) under --config=msan. MSan reads it at process start, so a # runfile is exactly the right shape: declared, cache-correct, and inside the sandbox. Public because diff --git a/tools/benchmark_history_test.py b/tools/benchmark_history_test.py index a85fdd5c32c..5ce984b3a32 100644 --- a/tools/benchmark_history_test.py +++ b/tools/benchmark_history_test.py @@ -233,23 +233,36 @@ def test_ci_shards_receive_the_prebuilt_native_resource_launcher(self): with self.subTest(workflow=workflow): source = repository_file(workflow).read_text().split('\n ' + job + ':', 1)[1] source = re.split(r'\n [a-z][a-z-]*:', source, maxsplit=1)[0] - self.assertIn('bazel cquery //tools:process_resources_native --config=clang_release', source) + self.assertIn("bazel cquery 'config(//tools:process_resources_native, target)' " + "--config=clang_release", source) self.assertIn('cp "${launcher}" "${RUNNER_TEMP}/benchmark-resource-launcher"', source) self.assertIn('tar -cf benchmark-build.tar -C "${RUNNER_TEMP}" ' 'benchmark-head benchmark-resource-launcher', source) - # The paired main driver already builds the helper through its data dependency. - self.assertIn('//tools:benchmark_resources' if job == 'build' - else '//tools:process_resources_native', source) + # A Python driver's data dependency can use a transitioned configuration. + # Build the exact target configuration queried and copied below. + head_build = source.split('bazel build ', 1)[1].split('\n', 1)[0].split() + self.assertIn('//tools:process_resources_native', head_build) + self.assertIn('--config=clang_release', head_build) + self.assertLess(source.index('bazel build '), + source.index('bazel cquery ')) action = repository_file('.github/actions/benchmark-measure/action.yml').read_text() self.assertIn('test -x "${RUNNER_TEMP}/benchmark-resource-launcher"', action) self.assertIn('--resource-launcher="${RUNNER_TEMP}/benchmark-resource-launcher"', action) self.assertNotIn('bazel build', action) self.assertNotIn('bazel run', action) + def test_native_launcher_queries_select_only_the_built_target_configuration(self): + for workflow in ('.github/workflows/main.yml', '.github/workflows/benchmarks.yml', + '.github/workflows/benchmark_backfill.yml'): + with self.subTest(workflow=workflow): + source = repository_file(workflow).read_text() + query = source.split('launcher="$(bazel cquery ', 1)[1].split('cp "${launcher}"', 1)[0] + self.assertIn("'config(//tools:process_resources_native, target)' --config=clang_release", query) + def test_ci_backfill_builds_native_launcher_before_freezing_and_measuring(self): source = repository_file('.github/workflows/benchmark_backfill.yml').read_text() build = source.index('bazel build //tools:process_resources_native --config=clang_release') - query = source.index('bazel cquery //tools:process_resources_native --config=clang_release') + query = source.index("bazel cquery 'config(//tools:process_resources_native, target)' --config=clang_release") copy = source.index('cp "${launcher}" "${RUNNER_TEMP}/benchmark-resource-launcher"') run = source.index('python3 tools/benchmark_campaign.py run') self.assertLess(build, query) diff --git a/tools/collect_compile_sources.sh b/tools/collect_compile_sources.sh new file mode 100755 index 00000000000..8ebc85dccf2 --- /dev/null +++ b/tools/collect_compile_sources.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) M. Boerger, the MBO Works authors +# SPDX-License-Identifier: Apache-2.0 + +# Emit only a complete set of successful first-party source queries. A headers-only +# query can legitimately be empty, but a failed query must not publish a partial list. +set -euo pipefail + +if [[ "$#" -eq 0 ]]; then + printf '%s\n' 'usage: collect_compile_sources.sh BAZEL_PATTERN...' >&2 + exit 2 +fi + +query_output="$(mktemp)" +collected="$(mktemp)" +trap 'rm -f "${query_output}" "${collected}"' EXIT + +for pattern in "${@}"; do + if ! bazel query "filter(\"\\.cc$\", kind(\"source file\", deps(kind(\"cc_.* rule\", ${pattern}))))" >"${query_output}"; then + printf '%s\n' "ERROR: source query failed for ${pattern}" >&2 + exit 1 + fi + # Unlike grep, awk succeeds when a valid query contains no matching sources. + # Genuine read/write errors still fail under errexit. + awk '$0 ~ "^@@(//|xff_)" { print }' "${query_output}" >>"${collected}" +done + +cat "${collected}" diff --git a/tools/collect_compile_sources_fake.sh b/tools/collect_compile_sources_fake.sh new file mode 100755 index 00000000000..184a1134aaa --- /dev/null +++ b/tools/collect_compile_sources_fake.sh @@ -0,0 +1,24 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) M. Boerger, the MBO Works authors +# SPDX-License-Identifier: Apache-2.0 + +set -euo pipefail + +if [[ "${1:-}" != query ]]; then + exit 2 +fi +case "${2:-}" in + *//first/...*) + printf '%s\n' '@@//xff/engine/run.cc' '@@third_party+//ignored.cc' + ;; + *//second/...*) + printf '%s\n' '@@xff_archive+//:backend.cc' + ;; + *//empty/...*) ;; + *//broken/...*) + printf '%s\n' '@@//xff/partial.cc' + printf '%s\n' 'controlled query failure' >&2 + exit 17 + ;; + *) exit 2 ;; +esac diff --git a/tools/collect_compile_sources_test.sh b/tools/collect_compile_sources_test.sh new file mode 100755 index 00000000000..6f6361610e3 --- /dev/null +++ b/tools/collect_compile_sources_test.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) M. Boerger, the MBO Works authors +# SPDX-License-Identifier: Apache-2.0 + +set -euo pipefail + +# shellcheck disable=SC1090,SC1091,SC2154 +source "${mboworks_bashtest}" + +collector="${TEST_SRCDIR}/${TEST_WORKSPACE}/tools/collect_compile_sources.sh" +fake="${TEST_SRCDIR}/${TEST_WORKSPACE}/tools/collect_compile_sources_fake.sh" + +_fixture() { + local fixture + fixture="$(test_tmpdir source-query)" + cp "${fake}" "${fixture}/bazel" + chmod +x "${fixture}/bazel" + printf '%s\n' "${fixture}" +} + +test::complete_queries_filter_third_party_and_allow_empty_packages() { + local fixture output rc + fixture="$(_fixture)" + output="$(PATH="${fixture}:${PATH}" "${collector}" //first/... //empty/... //second/...)" && rc=0 || rc=$? + expect_eq 0 "${rc}" + expect_eq $'@@//xff/engine/run.cc\n@@xff_archive+//:backend.cc' "${output}" +} + +test::a_failed_later_query_reports_the_pattern_and_emits_no_partial_list() { + local fixture output rc + fixture="$(_fixture)" + output="$(PATH="${fixture}:${PATH}" "${collector}" //first/... //broken/... //second/... 2>"${fixture}/error")" && rc=0 || rc=$? + expect_eq 1 "${rc}" + expect_eq '' "${output}" + expect_output_contains 'controlled query failure' "$(<"${fixture}/error")" + expect_output_contains 'source query failed for //broken/...' "$(<"${fixture}/error")" +} + +test::an_empty_successful_query_does_not_look_like_a_failure() { + local fixture output rc + fixture="$(_fixture)" + output="$(PATH="${fixture}:${PATH}" "${collector}" //empty/...)" && rc=0 || rc=$? + expect_eq 0 "${rc}" + expect_eq '' "${output}" +} + +test::missing_patterns_are_a_usage_error() { + local output rc + output="$("${collector}" 2>&1)" && rc=0 || rc=$? + expect_eq 2 "${rc}" + expect_output_contains 'usage:' "${output}" +}