fix: memoise key parsing in RubyDataParser to fix combiner performance regression#1239
Open
oleksii-leonov wants to merge 1 commit into
Open
fix: memoise key parsing in RubyDataParser to fix combiner performance regression#1239oleksii-leonov wants to merge 1 commit into
oleksii-leonov wants to merge 1 commit into
Conversation
…e regression Measured on a real 66-worker CI merge (~21k files, ~46k branches, branch coverage enabled): the collate step (which merged 66 reports) regressed from ~29s under `0.22` to ~145s (5x slower) under `1.0.0`. A wall-clock stackprof attributes main time to `Ripper#parse` under `RubyDataParser.parse_array_string`. Resultset JSON stores keys as stringified Ruby literals (`"[:if, 0, 14, 4, 14, 30]"`), and RubyDataParser walks them back into arrays with a Ripper parse. That parse sat on a hot path: `BranchesCombiner` and `MethodsCombiner` derive a merge identity from every key of *both* sides on *every* pairwise merge. Folding N resultsets therefore parsed each key string N-1 times. This PR adds memoisation to `RubyDataParser.call` to avoid parsing strings that have already been parsed. Notes: - The set of unique keys is bounded by the project's branch and method count, so the cache stays reasonably small and needs no eviction. - Cached arrays are frozen and shared. Every caller (both combiners, `BranchBuilder`, `MethodBuilder`, `SourceFile::Method`) destructures the parsed array without mutating it; freezing enforces that any future mutation surfaces as an error instead of silent cross-caller corruption. - Array inputs still pass through untouched and unfrozen - in-process results never hit the cache, exactly as before.
Contributor
There was a problem hiding this comment.
Pull request overview
Improves SimpleCov::SourceFile::RubyDataParser performance during large report collation by memoizing parsing of stringified tuple keys (avoiding repeated Ripper parses on hot merge paths), and adds tests + changelog entry to cover the new behavior.
Changes:
- Memoize
RubyDataParser.callstring parses via an internal cache and freeze cached tuple arrays. - Add specs asserting
.callbehavior for strings, passthrough arrays, and memoization. - Document the performance fix in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spec/source_file_spec.rb | Adds examples covering .call parsing, passthrough behavior, and memoization/freeze expectations. |
| sig/internal/simplecov/source_file/ruby_data_parser.rbs | Types the new memoization cache and parse_cache helper. |
| lib/simplecov/source_file/ruby_data_parser.rb | Introduces memoization in .call and a cache accessor. |
| CHANGELOG.md | Notes the combining/collate performance regression fix under “Unreleased”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+32
to
+33
| string = structure.to_s | ||
| parse_cache[string] ||= parse_array_string(string).freeze |
Collaborator
|
@oleksii-leonov Have you tested how this patch impacts the performance on your 66-worker CI merge? |
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.
Measured on a real 66-worker CI merge (~21k files, ~46k branches, branch coverage enabled): the collate step (which merged 66 reports) regressed from ~29s under
0.22to ~145s (5x slower) under1.0.0.A wall-clock stackprof attributes main time to
Ripper#parseunderRubyDataParser.parse_array_string.Resultset JSON stores keys as stringified Ruby literals (
"[:if, 0, 14, 4, 14, 30]"), andRubyDataParserwalks them back into arrays with a Ripper parse. That parse sat on a hot path:BranchesCombinerandMethodsCombinerderive a merge identity from every key of both sides on every pairwise merge. Folding N resultsets therefore parsed each key string N-1 times.This PR adds memoisation to
RubyDataParser.callto avoid parsing strings that have already been parsed.Notes:
BranchBuilder,MethodBuilder,SourceFile::Method) destructures the parsed array without mutating it; freezing enforces that any future mutation surfaces as an error instead of silent cross-caller corruption.