Repository navigation
Avoid temporary key construction and retain executor audit evidence - #1037
Merged
Merged
Conversation
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
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.
Avoid unnecessary temporary strings when recording collection names and comparison categories while preserving owned keys. Retain the complete cross-platform executor/copy audit, including useful memory gains and counterexamples, so future optimization work can build on reproducible evidence.
AG;DR
The implementation retains only C01/C03 from the copy audit:
Collections::Adduses transparenttry_emplace(name)instead of constructingstd::string(name)for every lookup. New keys remain owned; the regression test mutates/destroys caller storage and checks repeated-name lookup and retained entries.string_viewand constructs an owned map key only when retention is needed, avoiding the eagerstd::string(pair.path)temporary.a49c17045dcandidate. The regression test has matcher-onlyEqualsTextstyle corrections and the direct matcher dependency; the immutable archive retains the original measured source/test patches. No recycler API, executor integration, admission gate or scheduling behavior is added.The new report and condensed performance-analysis entry cover all seven original PRs (#991, #992, #996, #997, #998, #1000, #1001), actual dependencies, isolated/cumulative variants and eager mechanism controls. Keep main's scheduling: useful Linux gains coexist with macOS collection and specific content regressions. Removing the inherited 8192-entry/500us matching guard repairs the largest content regressions on both platforms, without establishing a universal shared-pool win. The bounded frontier has a demonstrated case-specific memory benefit: in the 65,536-file metadata case at three workers, #997 reduces median peak RSS from 20.06 to 6.25 MiB on Linux and 17.34 to 5.44 MiB on macOS (about 69% vs #996); this is not a worst-case bound. C02 recycling remains deferred. C01/C03 are narrow construction/ownership fixes, with no universal wall-clock promise.
The lossless
docs/benchmarks/executor-copy-audit-20261101.json.gzarchive contains ten complete report populations, fifteen full binary-capable source patches, the earlier #991 reassessment, raw samples, drivers/fixtures and provenance corrections. Its exact size is 4,351,250 bytes; SHA256 isb10a92105d69154e933ccfa4f5bb1776ad27e145c80e9b9f8de976da259c7560. The initial native matrix has 53 cases, three sessions and 15,900 oracle-correct invocations per platform, followed by separate native/in-memory controls. The report preserves CPU/storage differences, original manifest labels with explicit tree-equivalence correction, full source identities and negative controls. Collector Google Benchmark threads are distinguished from engine job budgets; its short-key/checksum contract is weaker than native complete-output oracles.Register the report in
release-site.json; add the allocation note to CHANGELOG and remaining qualification work to TODO. The archive exceeds the normal 500 KB pre-commit limit, socheck-added-large-fileshas one exact-filename exception for this evidence artifact; other hook checks remain enabled.Validation: current pushed head
542ca47120e8ded422e88fc9de66972f45e7ceb0includes current main8ff28cde9c67bd498f9016935da01506ffb5845c. CI run41307562869completed SUCCESS; all 32 reported contexts are successful or skipped, including both default platform suites, ASan/TSan/MSan, GCC/minimal, clang-tidy, coverage/fuzz, release-site, six benchmark measurement shards, cross-platform comparison and finaldone. Normal commit/push hooks passed; the synchronization push ran actual clang-tidy on all five inherited infrastructure translation units after a scoped compile-database refresh. Focused collection/matcher/rg/run tests previously passed on both platforms for the measured copy candidates, and the matcher-only test correction received fresh focused collection validation. The six-translation-unit audit lint covered retained C01/C03 code in the recycler superset rather than a separate a49-only lint run. Raw archive populations/source patches and error-free retained results were independently checked. These measurements do not validate unmodified runtime PR heads.MSan timeout correction:
//xff/cli:rg_testaggregates 33 functional cases with repeated frontend subprocesses. An earlier run passed 297/298 tests but this test hit its implicit 60-second small-test timeout, without a sanitizer diagnostic or failed assertion; an earlier successful infrastructure run took 55.3 seconds. Set only its timeout tomoderate(300 seconds), preserving sizesmalland every assertion/case. No application code, skipping, splitting or sanitizer suppression is introduced. Focused macOS release rg_test passed in 3.8 seconds; the corrected pre-sync Linux MSan run passed all 299 tests, with rg_test taking 68.5 seconds, exceeding the old limit. The final synchronized head also passes full MSan in its own CI context.Portability/limits: release measurements use Clang 23.1.4/ThinLTO, Linux KVM/tmpfs with CPU affinity and native Apple M5 Pro/APFS without affinity. In-memory Google Benchmark results use a separate contract. Small timing changes and byte-identical negative controls do not establish causality or significance; physical slow storage, arbitrary worst-case RSS and full-plugin/PCRE2 startup are not established.
Dependencies: based on current main, including independently merged infrastructure #1035. That PR's synchronized head
b830e17874completed full CI run41303081236before the user merged it as8ff28cde9c. Its test-only support has no shipping executor dependency. Post-merge main Test run41307382327completed SUCCESS, including default macOS and finaldone. The separate main Benchmarks workflow41307382275is in progress. These are separate contexts from this fully green PR.Concurrent #1036 was independently classified as an isolated uploader inventory/display component: one read-only published batch/status scan plus set intersections outside the CLI/measurement loop. Independently merged #1038 adds uploader dataset-recipe overlap/incomplete-batch inventory and bounded publication rebase/regenerate/retry in Python/workflow/docs. Neither changes C++ runtime, benchmark measurement execution or retained raw samples; no additional timing is needed for these runtime claims. The immutable audit baseline remains historical main
974d137026, preserving measured identities rather than relabeling results after synchronization. Original runtime PR heads remain unchanged.Ready for review only. No approval, merge or auto-merge is requested.