perf: resolve RSpec test file paths without allocating per example#344
Open
connorshea wants to merge 1 commit into
Open
perf: resolve RSpec test file paths without allocating per example#344connorshea wants to merge 1 commit into
connorshea wants to merge 1 commit into
Conversation
`RSpecAdapter.file_path_for` runs for every test example and allocated an array of four lambdas on every call, purely to check its candidate paths lazily. Checking them inline keeps the same order and the same laziness. `parse_file_path` and `id_path?` built a MatchData for every path - plus, because the target string is unfrozen, a frozen copy of it, and an array from `captures`. An id path always ends with `]` and the id characters can never contain `[`, so the only possible split point is the last `[` of the path. `parse_file_path` now finds it with `rindex` and validates it with an allocation-free `Regexp#match?` starting at that offset; `id_path?` is a plain `match?`. Verified equivalent to the old regexp on 550 hand-picked paths and 50k fuzzed inputs over the interesting alphabet, including paths whose file name itself contains brackets and trailing brackets that are not ids. Those cases are now covered by specs. file_path_for 13 allocations / 2.1 us -> 1 / 0.46 us parse_file_path 6 allocations / 0.97 us -> 0 / 0.10 us (no id) id_path? 6 allocations / 1.05 us -> 0 / 0.09 us (no id) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Description
AI Disclosure: This was generated using Claude Code with Opus 5. It has been reviewed and tested by me manually.
RSpecAdapter.file_path_forruns on every test example (in Queue Mode, it runs twice per example) and was allocating more objects than necessary to check candidate paths. We can instead check it inline while still keeping the laziness.We built a MatchData and a few other things for every call of
parse_file_pathandid_path?. We don't need to do that here, because we can instead look for the last[in the path and then match on the string from there onward, without needing to allocate almost anything.parse_file_pathnow finds[withrindexand validates it with an allocation-freeRegexp#match?starting at that offset, andid_path?is now anend_with?+match?.The only functional change in this code is that a path with a linebreak (which shouldn't happen anyway) would previously cause a NoMethodError, and is now ignored/kept instead. I don't think that's a problem because it doesn't ever happen, but figured I'd note it.
Also wanted to note that I had Claude fuzz the new regex to ensure equivalent behavior on 250k values (some generated to match real paths, some fully random values).
This saves a tiny bit of time, but the main benefit of this change is reducing the # of object allocations because it helps avoid triggering GC in large suites, which speeds things up. This isn't going to be hugely significant on its own, but I think it's worth doing.
Per-call, Ruby 3.4.9, arm64-darwin25, 500 RSpec example ids,
benchmark-ips(3s, 1s warmup)file_path_forparse_file_path(id path)parse_file_path(plain path)id_path?(id path)id_path?(plain path)Scaled to a suite (one
file_path_forper example, NOT Queue Mode)Scaled to a suite (Queue Mode: two
file_path_forcalls per example)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.