fix: concurrent callers each spawned their own git rev-parse - #253
Conversation
The per-root cache added in #250 removed 200 process spawns from a 200-screenshot serial suite, but bought nothing under threads. MRI releases the GVL for the whole of `Open3.capture3`, so every thread misses `key?` before any thread writes: measured 8 spawns for 8 threads asking about a single root. That is the default parallel mode on JRuby, which additionally has no GVL to make the unsynchronized Hash write safe. Synchronize the lookup itself, not just the write. Holding the lock across the spawn is deliberate -- callers almost always share one root, so the other threads wait once and then read the cache.
|
Warning Review limit reachedNext included review available in 44 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideSynchronizes git root caching in SnapDiff::Vcs so concurrent thread callers share a single git rev-parse invocation, and adds a unit test that exercises and mutation-checks the concurrent behavior via a stubbed capture3. Sequence diagram for synchronized git root lookupsequenceDiagram
participant T1 as Thread 1
participant T2 as Thread 2
participant Vcs as SnapDiff::Vcs
participant Git as Open3.capture3
par Concurrent callers
T1->>Vcs: git_root_for(root_path)
T2->>Vcs: git_root_for(root_path)
end
Vcs->>Vcs: @git_roots_lock.synchronize
Vcs->>Git: Open3.capture3(git, -C, root_path, rev-parse, --show-toplevel)
Git-->>Vcs: git_root, status
Vcs->>Vcs: @git_roots[root_path] = status.success? && git_root.chomp
Vcs-->>T1: cached git root
Vcs-->>T2: cached git root
Vcs->>Vcs: @git_roots_lock.synchronize
Vcs-->>Vcs: @git_roots.key?(root_path)
Flow diagram for concurrent git root cache behaviorflowchart TD
A[Concurrent git_root_for calls] --> B{Cache lookup under mutex}
B -->|miss| C[Open3.capture3 git rev-parse]
C --> D[Store result in @git_roots]
D --> E[Return cached git root]
B -->|hit| E
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
The v2.0.0 section was written before #250, #253, #254, #255, #256, #261, #263, #264, #266 and #267 landed, and three of its claims had gone false: - "Known limitations: fork-based parallel tests produce no HTML report ... Fixed in 2.1" -- fixed in 2.0 by #266. Reproduced both sides here: 1.15.1 + `parallelize(workers: 2, threshold: 0)` writes NO report and prints no summary line; master writes one merged report and `4 verified, 4 changed, 0 new`. - "a suite whose only contact with the v1 API is `require \"capybara_screenshot_diff/minitest\"` + `include ...Assertions` still prints nothing" -- #263 made the require doors warn. That exact setup now prints the migration notice; verified in a scratch project. - "Two removals 2.0 cannot warn about ... `driver:` as a setting" -- #263 made both the setting writer and the per-screenshot key warn. Verified: `Capybara::Screenshot::Diff.driver = :vips` prints the removal line with a call site. And the silent-by-design constant list repeated the shape of the beta2 `defined?` mistake: it listed "Os, Region" inside a run of `Capybara::Screenshot::Diff::` names. Probed on master -- `defined?(Capybara::Screenshot::Diff::Os)` and `defined?(Capybara::Screenshot::Diff::Region)` are both nil. The real names are `Capybara::Screenshot::Os` and the top-level `Region`, neither of which existed under `::Diff` in 1.15.1 either. Fully qualified now, and `::Comparison` added to match docs/UPGRADING.md. New material, every claim checked against the code or a live run: - a "why upgrade" section for the four green-suite-testing-nothing bugs (#255, #256, #254, #266), plus the unfollowable CI message (#267) and the fail_if_new precedence change - before/after transcripts of the failure message (#264), taken from the same page rendered on 1.15.1 and on master - the summary line (#261), with the fact that it comes from the HTML reporter and needs its one-line require -- an omission that would have read as a missing feature - the #250 / #253 perf table, attributed to its harness, with columns labelled before/after rather than 1.x/2.0 - the libvips fix is stated as guarded on libvips 8.15+, so a reader on an older libvips knows the bug is still theirs Install snippets stay pinned to 2.0.0.beta3 on purpose: `~> 2.0` resolves to nothing on rubygems today. docs/RELEASE_PREP.md already carries a precise step to swap all five (its grep finds exactly those five), and gains one line so the record-modes placeholder in the entry cannot ship unfilled. `rake test:unit` 651 runs / 0 failures, `standardrb lib test` clean.
Follow-up to #250, found while auditing parallel test execution.
#250 caches
git rev-parse --show-toplevelper root, removing ~200 process spawns from a 200-screenshot suite. Under threads it removed none.MRI releases the GVL for the duration of
Open3.capture3, so concurrent callers all misskey?before any of them writes. Measured on master, 8 threads asking about one root:That is the cost the cache exists to remove, still fully paid — in
parallelize(with: :threads), which is the default parallel mode on JRuby. JRuby also has no GVL, so the unsynchronizedHash#[]=is unsafe there as well as wasteful.The fix synchronizes the lookup rather than the write, so the spawn happens once. Holding the lock across the subprocess is deliberate: callers almost always share a single root, so the remaining threads wait once and then read the cache.
After:
git spawns for ONE root, 8 concurrent callers: 1Guard:
test/unit/vcs_test.rbdrives 8 concurrentcheckout_vcscalls through a stubbedcapture3that sleeps (standing in for the GVL-releasing subprocess) and asserts exactly one lookup. Mutation-checked — replacing the mutex with a lock that merely yields reds the test; restored, green.588 unit runs / 0 failures, standardrb clean.
Summary by Sourcery
Synchronize Git-root caching so concurrent VCS operations share a single repository lookup.
Bug Fixes:
git rev-parseprocesses when resolving the same repository root.Tests: