perf(ingestion): one repo walk per update, not one per worker - #1926
Open
azhard wants to merge 5 commits into
Open
perf(ingestion): one repo walk per update, not one per worker#1926azhard wants to merge 5 commits into
azhard wants to merge 5 commits into
Conversation
added 3 commits
August 25, 2026 18:55
`build_repo_graph` maps `_build_file_info` over every path with ~2x cpu_count workers, and that method reaches an unsynchronised lazy init in `_console_script_tables`. Every worker arriving before the first one assigns starts its own `_collect_console_scripts`, each a full `iter_glob` walk of the repo. Measured 263.67s against 8.06s when the value is computed once, for identical FileInfos. Double-checked locking there, and the same treatment for the `_get_dir_ignore` cache on the same hot path. `_primary_language_in` and `_find_entry_points_in` walked each package separately and neither applied the traverser's ignore semantics, so both descended into gitignored build output — where language detection opens every file it cannot classify by extension. They become one `_scan_package_dir` taking the `is_pruned` predicate `scan_package_roots` already uses. That also stops `target/criterion` reports outvoting Rust sources for a bench crate's language. Four resolver scans route through the existing `glob_via` seam so they share `ResolverContext.walk_snapshot` instead of walking the repo again. Graph output is unchanged: same nodes, same edges, edge sets equal.
…shot The concurrency tests fail without the locks: dropping the double-checked lock makes `_collect_console_scripts` run once per worker, and the plain `cache[key] = ...` / `return cache[key]` pair lets two threads hand back different specs for one directory. `test_pre_seeded_root_entry_survives_concurrent_readers` passes either way. It is not a regression test — it pins the root entry `__init__` seeds, which `setdefault` keeps and a bare assignment would have clobbered. The cmake pair covers the orphan sweep: one that a snapshot answers with the same files as a live walk, one that a snapshot built with `prune_nested_git=False` surfaces a nested checkout the default hides. That second case is the behaviour change in this branch, so it gets a test rather than a mention.
The scan's own claim is that a package is described by the sources traversal indexes, and nothing exercised it. Four cases: a gitignored output dir no longer outvoting the real source, entry points coming only from indexed directories, a *committed* `dist/` excluded too because `_BLOCKED_DIRS` means the traverser never indexed it either, and the one shape that gets less informative — a package whose sources are all ignored now reports `unknown`. Each asserts against `is_pruned=None`, which is what the two old helpers did, so they fail if the pruning is dropped.
|
✅ Health of changed files: 4.9 (unchanged) 📋 At a glance Files & modules (2)
✅ Health gate: passed 📌 Before you merge
🎯 Blast radius (symbols whose signature this PR changed, and who calls them)
🔎 More signals (2)🗺️ Change map flowchart LR
subgraph PR ["Changed in this PR (6 with dependents)"]
f_packages_core_src_repowise_core_ingestion_external_systems_cmake_py[".../external_systems/cmake.py 🔥"]:::changed
f_packages_core_src_repowise_core_ingestion_resolvers_ruby_py[".../resolvers/ruby.py 🔥"]:::changed
f_packages_core_src_repowise_core_ingestion_resolvers_swift_spm_py[".../resolvers/swift_spm.py 🔥"]:::changed
f_packages_core_src_repowise_core_ingestion_traverser_py[".../ingestion/traverser.py 🔥"]:::changed
f_packages_core_src_repowise_core_ingestion_resolvers_cpp_workspace_py[".../resolvers/cpp_workspace.py"]:::changed
f_tests_unit_ingestion_test_traverser_py[".../ingestion/test_traverser.py"]:::changed
end
f_packages_core_src_repowise_core_ingestion_external_systems___init___py[".../external_systems/__init__.py"]
f_packages_core_src_repowise_core_ingestion_external_systems_cmake_py --> f_packages_core_src_repowise_core_ingestion_external_systems___init___py
f_packages_core_src_repowise_core_ingestion_resolvers___init___py[".../resolvers/__init__.py"]
f_packages_core_src_repowise_core_ingestion_resolvers_ruby_py --> f_packages_core_src_repowise_core_ingestion_resolvers___init___py
f_packages_core_src_repowise_core_ingestion_graph__resolvers_py[".../graph/_resolvers.py"]
f_packages_core_src_repowise_core_ingestion_resolvers_swift_spm_py --> f_packages_core_src_repowise_core_ingestion_graph__resolvers_py
f_packages_core_src_repowise_core_ingestion_resolvers_swift_py[".../resolvers/swift.py"]
f_packages_core_src_repowise_core_ingestion_resolvers_swift_spm_py --> f_packages_core_src_repowise_core_ingestion_resolvers_swift_py
f_packages_cli_src_repowise_cli_commands_dead_code_cmd_py[".../commands/dead_code_cmd.py"]
f_packages_core_src_repowise_core_ingestion_traverser_py --> f_packages_cli_src_repowise_cli_commands_dead_code_cmd_py
f_packages_cli_src_repowise_cli_commands_update_cmd_persistence_py[".../update_cmd/persistence.py"]
f_packages_core_src_repowise_core_ingestion_traverser_py --> f_packages_cli_src_repowise_cli_commands_update_cmd_persistence_py
f_packages_core_src_repowise_core_analysis_dead_code_analyzer_py[".../dead_code/analyzer.py"]
f_packages_core_src_repowise_core_ingestion_traverser_py --> f_packages_core_src_repowise_core_analysis_dead_code_analyzer_py
f_packages_core_src_repowise_core_analysis_decisions_extractor_py[".../decisions/extractor.py"]
f_packages_core_src_repowise_core_ingestion_traverser_py --> f_packages_core_src_repowise_core_analysis_decisions_extractor_py
f_packages_core_src_repowise_core_ingestion_call_resolver_py[".../ingestion/call_resolver.py"]
f_packages_core_src_repowise_core_ingestion_resolvers_cpp_workspace_py --> f_packages_core_src_repowise_core_ingestion_call_resolver_py
f_packages_core_src_repowise_core_ingestion_graph_warmups_py[".../ingestion/graph_warmups.py"]
f_packages_core_src_repowise_core_ingestion_resolvers_cpp_workspace_py --> f_packages_core_src_repowise_core_ingestion_graph_warmups_py
f_tests_unit_ingestion_test_traverser_py --> f_packages_cli_src_repowise_cli_commands_dead_code_cmd_py
more(["+11 more dependents"])
PR --> more
t_tests_unit_ingestion_test_cpp_include_fragments_py(["✅ .../ingestion/test_cpp_include_fragments.py"]):::guard
t_tests_unit_ingestion_test_cpp_include_fragments_py -.-> f_packages_core_src_repowise_core_ingestion_external_systems_cmake_py
t_tests_unit_ingestion_test_ruby_resolver_py(["✅ .../ingestion/test_ruby_resolver.py"]):::guard
t_tests_unit_ingestion_test_ruby_resolver_py -.-> f_packages_core_src_repowise_core_ingestion_resolvers_ruby_py
t_tests_unit_ingestion_test_swift_resolver_py(["✅ .../ingestion/test_swift_resolver.py"]):::guard
t_tests_unit_ingestion_test_swift_resolver_py -.-> f_packages_core_src_repowise_core_ingestion_resolvers_swift_spm_py
classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d
Solid arrows: code that imports the changed files (21 direct dependents, from the last indexed snapshot). Dashed: history/tests. 🔥 Hotspots touched (5)
2 more
👀 Suggested reviewers @RaghavChamadiya 📊 See the full report for this PR |
added 2 commits
August 25, 2026 19:47
…ng it The bot flagged two new findings on `_scan_package_dir`, and a reviewer had separately called the `is_pruned=None` default dead flexibility — the one caller always passes `dir_chain_skipped`. Both are the same thing: an optional predicate buys a branch nothing takes, and a scan that skips it answers from files nothing indexes, which is the bug this function exists to fix. Required now, so the branch goes with it. `complex_method` clears; `nested_complexity` stays, and extracting the inner loop to chase it would trade one shape of complexity for a mutation-heavy helper. The tests that used `None` to mean "what the two old helpers did" say it outright with a `_never_pruned` predicate.
`test_a_committed_build_dir_is_excluded_too` gave the package one `.ts` and one `.js`, so `counts` was a 1-1 tie and `max(counts, key=...)` returned whichever language came first. `walk_repo` does not sort dirnames, so that is filesystem order: `dist` first here, `src` first on CI, and the assertion flipped between them. Five bundles instead of one, so the unindexed directory outnumbers the real source and the pre-fix answer is `javascript` either way. Checked by forcing both orders: the old shape yields javascript/typescript, the new one yields javascript twice. The sibling gitignored-output test already used five, which is why it passed on both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_build_file_inforan one full repo walk per worker.build_repo_graphmaps it over every path with ~2x cpu_count workers, and it reachesFileTraverser._console_script_tables(), whose lazy init is an unsynchronisedif self._console_scripts is None. Every worker arriving before the first one assigns starts its own_collect_console_scripts— each a fulliter_globwalk. Measured on a 17k-file repo with 28 workers: 263.67s vs 8.06s when the value is computed once, for identicalFileInfooutput. Fixed with double-checked locking, plus the same treatment for the_get_dir_ignorecache on the same hot path (functools.cached_propertyis not a substitute — 3.12 dropped its internal lock)._primary_language_inand_find_entry_points_inwalked each package separately, and neither applied the traverser's own ignore semantics._BLOCKED_DIRSalready containsdist/build/target/vendor, so those directories are never indexed — committed or not — yet they could decide a package's language. A committeddist/bundle.jsthat is not gitignored and not indexed made a TypeScript package reportjavascript. Merged into one_scan_package_dirtaking theis_prunedpredicatescan_package_rootsalready uses: 30.89s → 1.12s, and the answers get corrected (on the repo I measured, a bench crate stopped reportingdlangfrom itstarget/criterionHTML, and package entry points went 1,452 → 11, all 1,441 dropped paths untracked).GraphBuilder.build(), costing 339,025realpathcalls and 4.1Mlstats. Four now route through the existingglob_viaseam and shareResolverContext.walk_snapshot. Net: ingestion pipeline 322.73s → 26.36s,repowise update --since HEAD~18m41s → 3m27s, with the graph unchanged (77,493 nodes / 215,904 edges, edge sets equal element by element).One deliberate behaviour change:
discover_cmake_reactor's orphan sweep never took aprune_nested_gitargument, so it hardcodedTrueand ignored the repo's setting. Served from the snapshot it now inheritsctx.prune_nested_git, so a repo indexed with--include-nested-repossurfacesCMakeLists.txtfiles under nested checkouts that were previously skipped. This brings cmake in line with whatruby.pyandswift_spm.pyalready did, and it has a test either way. Flagging it because it is the one thing here that changes output rather than only cost.One case gets less informative, deliberately: a package whose sources are all ignored now reports
unknownrather than a language read off files nothing indexes.Related Issues
Refs #327 — the "post-commit hook extremely slow" thread, where the diagnosis was "the incremental update path still re-traverses and reparses the repo so even a small commit can make the background update take a while." This is that, measured and fixed. No open issue tracks it.
Test Plan
uv run pytest tests/unit/ingestion tests/unit/pipeline— 2711 passed, 5 skipped. One failure,test_repo_totals_churn.py::test_a_rebased_branch_in_the_history_falls_back, reproduces on a pristine checkout of the same commit (itsgit rebase -q masterfails whereinit.defaultBranchismain) and is unrelated to these files.uv run pytest tests/unit/ingestion/test_traverser.py tests/unit/ingestion/external_systems/test_cmake.py— 114 passeduv run ruff check— clean on all changed filesuv run repowise risk main..HEAD— "about as risky as a typical commit in this repo", 47th percentilenpm run build) (no frontend changes)Nine tests added, and I checked which of them actually fail without the fix rather than assuming:
test_console_script_tables_is_collected_once_collect_console_scriptsruns once per workertest_dir_ignore_cache_publishes_one_spec_per_directorytest_pre_seeded_root_entry_survives_concurrent_readerssetdefaultkeepstest_gitignored_output_no_longer_decides_the_languagetest_entry_points_come_only_from_indexed_directoriestest_a_committed_build_dir_is_excluded_tootest_a_package_with_nothing_indexed_reports_unknowntest_orphan_glob_reads_the_snapshot_when_given_onetest_orphan_glob_honours_the_snapshot_nested_repo_settingChecklist
Rejected alternatives
build/dist/outinPRUNED_DIRS.fs_walk.pysays explicitly not to use the derived list for manifest discovery, "where a module legitimately rooted atbuild/orcoverage/must still be found". The traverser's own ignore test is the right boundary and already exists.WalkSnapshot.walk()method sots_workspace._get_repo_scancould share too — that needs an API addition rather than an existing seam, so it is left out.Not fixed here
Four repo walks remain per build.
read_go_modulesruns before theResolverContextexists, so sharing the snapshot means threading it down fromGraphBuilder;ts_workspace._get_repo_scanuseswalk_repodirectly and needs the method above. Happy to follow up on either if you want them in scope.