Accumulate TimeTracker paths in-place instead of rebuilding the hash#343
Open
connorshea wants to merge 3 commits into
Open
Accumulate TimeTracker paths in-place instead of rebuilding the hash#343connorshea wants to merge 3 commits into
connorshea wants to merge 3 commits into
Conversation
… hash `example_group_finished` merged the finished test file's paths into the run-wide accumulator with `Hash#merge`, which builds a whole new hash. It runs once per top level group, so the work is quadratic in the number of test files a CI node executes. Accumulate into the existing hash instead. The entries are private to the formatter and their `:path` already equals the key they are stored under, so mutating the recorded time in place is equivalent. Driving the formatter with 2000 test files of 4 examples each: 21 ms and 74 MB of transient hash-table churn -> 15 ms and 2.0 MB. The merge is the whole quadratic term; isolated, it costs 0.4/1.7/5.8/20.2 ms at 500/1000/2000/4000 files, roughly 4x per doubling. With split-by-test-example the accumulator is keyed per example rather than per file, so it is worse there: 1000 files x 20 examples goes from 74 ms and 404 MB to 46 ms and 7.6 MB. Object allocation counts barely move (`merge` allocates one hash per call); the win is bytes and time, not object count. `merge` is still used by `#queue`, which is called once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`record_example` right above already branches on `accumulator.key?(path)`. The truthiness check was equivalent here, but only because `@paths` is a plain hash whose values are always hashes: on a `Hash.new(0)` accumulator (as `@time_all_by_group_id_path` is) a missing key returns `0`, which is truthy, and `0[:time_execution] +=` would raise. No measurable cost. On a collision-heavy workload (500 files x 20 top level groups, so 9.5k of 10k merges take the collision path) the extra lookup is lost in the noise: 81.0/81.1/81.5 ms before, 79.7/80.1/81.5 ms after, with identical allocations and identical recorded times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The entry was missing the `(patch)` prefix the repo uses to mark the needed version bump on unreleased entries. Also correct the wording: the merge runs once per top-level example group, not once per test file, so a spec file with several top-level `describe` blocks rebuilt the collection several times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
I think the failures are due to the tests needing to either be updated for simplecov 1.x, or we need to restrict the range of the simplecov gem |
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.
Description
AI Disclosure: This change was generated using Claude Code, with Opus 5. I have manually reviewed and tested the change for correctness and to ensure it makes sense.
The
example_group_finishedmethod in the TimeTracker formatter usedHash#mergeto add the finished test's paths into the accumulator, which meant building a whole new hash. This is run once per top-level group in RSpec, and so the work was being done repeatedly and scaled with the number of top-leveldescribeblocks as well as how many paths have already been added to the hash. This PR modifies the accumulator in-place instead to avoid adding memory pressure / triggering GC.I don't believe this change violates the architecture outlined in the PR template in any way (or at least it doesn't newly violate it, as the formatter already mixed purity and side-effects before this change).
These benchmarks were run on my M5 Pro Mac 24GB RAM w/ Ruby 3.4.9.
Per-file keys (if not splitting by example), 4 examples per file:
Split-by-test-example (
@pathskeyed per example — the worst case, but only really gets hit if there are a lot of slow tests):Checks
UNRELEASEDsection of theCHANGELOG.md, including the needed bump (i.e., patch, minor, major)lib/knapsack_pro/pure/queue/rspec_pure.rbcontains pure functions that are unit tested.lib/knapsack_pro/extensions/rspec_extension.rbencapsulates calls to RSpec internals and is integration and E2E tested.lib/knapsack_pro/runners/queue/rspec_runner.rbinvokes the pure code and the extension to produce side effects, which are integration and E2E tested.