fix: memoise key parsing in RubyDataParser to fix combiner performance regression

[oleksii-leonov]

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).

[Copilot]

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.call string parses via an internal cache and freeze cached tuple arrays.
• Add specs asserting .call behavior 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.

[sferik]

@oleksii-leonov Have you tested how this patch impacts the performance on your 66-worker CI merge?

[oleksii-leonov]

@sferik

Yes, we are using this branch in our CI.

Merge time dropped from ~150 seconds to ~35 seconds (roughly the same as it was for 0.22).

[oleksii-leonov]

I will check Copilot comment and rebase this PR (there is a conflict in CHANGELOG.md).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

lib/simplecov/source_file/ruby_data_parser.rb:35

• The cache key is currently derived as string = structure.to_s and used directly as a Hash key. If structure is a mutable String (e.g., JSON-parsed keys), mutating it later would corrupt Hash key semantics. The adjacent comment also incorrectly states that Hash#[]= duplicates and freezes String keys automatically. Use a frozen copy of the string as the cache key (and update the comment accordingly).

      # across callers must stay that way. The cache key needs no such
      # care — `Hash#[]=` dups and freezes String keys on its own.
      def call(structure)
        return structure if structure.is_a?(Array)

        string = structure.to_s
        parse_cache[string] ||= parse_array_string(string).each(&:freeze).freeze

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

lib/simplecov/source_file/ruby_data_parser.rb:31

• The comment says Hash#[]= duplicates/freezes String keys, but Ruby Hash does not do that. Since call can receive a mutable String (e.g. from JSON), using it directly as the cache key means a later mutation of that String could corrupt the Hash. Consider freezing/deduplicating the key (Ruby 3.2+ supports unary - on String) and update the comment accordingly.

      # across callers must stay that way. The cache key needs no such
      # care — `Hash#[]=` dups and freezes String keys on its own.
      def call(structure)

[sferik]

Approved. I will merge as soon as the CHANGELOG conflict is resolved.

[oleksii-leonov]

@sferik , the conflict was resolved. Feel free to merge.

I have a second proposal with some more refactoring (moving from pairwise merge into one-pass merge using a single accumulator instance). In our CI, it cut the time almost in half again (~35 seconds to ~19 seconds; time left is mainly JSON parsing).

[sferik]

@oleksii-leonov I prefer to "Rebase and merge" and GitHub still says “This branch cannot be rebased due to conflicts”. To fix that, you’ll need to do the following:

git fetch upstream # your remote for simplecov-ruby/simplecov
git checkout perf/memoize-coverage-key-parsing
git rebase upstream/main

The rebase will stop on your first commit with a conflict in CHANGELOG.md. Please keep both sides. Then:

git add CHANGELOG.md
git rebase --continue
git push --force

When do you expect your second PR will be ready? I was planning to push v1.0.3 tomorrow, but I’m happy to wait for your second patch to land.

Thanks so much for making these performance improvements!

[oleksii-leonov]

Oh, got it. I will make a clean rebase and squash it into 1 commit.

[oleksii-leonov]

@sferik , rebase and squash done.

When do you expect your second PR will be ready? I was planning to push v1.0.3 tomorrow, but I’m happy to wait for your second patch to land.

I would say let's merge/publish this PR first (it's simple and solves the main regression issue).

The second change is more complicated (60a36ee). Specs are green, and results on our codebase are identical. I am already using it in our CI to battle test. So I would prefer to merge/publish it separately.

[sferik]

@oleksii-leonov Thanks for the contribution! This change shipped in v1.0.3.

I also took a look at 60a36ee and it looks mostly good. I verified the fold-vs-accumulator equivalence. A couple things before you open a PR:

1. I found one parity bug. When the executed-side-is-authoritative rule engages, the fold substitutes NO_SYNTHESIZED (empty hashes), so the merged file always keeps at-least-empty "branches" and "methods" tables. The accumulator leaves them nil in both drop paths (drop_synthesized, and the elsif @executed early return for an incoming simulated result). When no executed result carries a "methods" key (criteria drift between workers), the merged resultset then contains "methods": null, which crashes report building in MethodBuilder (fetch("methods", none) returns the nil for a present key, then nil.filter_map raises). You should be able to fix this by assigning empty tables in drop_synthesized and ensure them before the early return.

2. Please add tests. At minimum the simulated and executed orderings through FileAccumulator and ideally a differential spec fuzzing the fold against the accumulator.

Happy to review the PR once these are addressed!

[oleksii-leonov]

@sferik , thank you a lot!

Regarding 60a36ee , I will try to shape it better in the following weeks (currently it's a draft).