JBTM-4038 JGroupsSlots and JGroupsRaftSlots benchmarks - #239
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds JGroups and JGroups Raft configurations, Maven dependency wiring, three JMH slot-store benchmarks, transaction rollback handling, report filtering, slot-count headroom, and benchmark CSV row-count validation. ChangesJGroups benchmark support
Sequence Diagram(s)sequenceDiagram
participant JMHRunner
participant RaftChannels
participant JGroupsRaftSlots
participant Leader
participant Follower
JMHRunner->>RaftChannels: create and connect three channels
RaftChannels->>RaftChannels: elect leader
JMHRunner->>JGroupsRaftSlots: initialize node slot stores
Leader->>Follower: replicate pre-populated slots
JMHRunner->>Follower: readFromFollower()
JMHRunner->>Leader: writeToLeader()
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/benchmark.sh:
- Line 113: Update BM5’s JMH selector pattern so it matches the newly added
JGroupsSlotsBenchmark and JGroupsRaftSlotsBenchmark benchmarks, including their
testJGroupsSlotsStore and testJGroupsRaftSlotsStore methods, while retaining the
existing StoreBenchmark matches and expected count of 7.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6999bbdf-b1be-4c03-ae87-b1357153804c
📒 Files selected for processing (6)
.github/scripts/benchmark.shnarayana/ArjunaJTA/jta/etc/jgroups-raft.xmlnarayana/ArjunaJTA/jta/etc/jgroups.xmlnarayana/ArjunaJTA/jta/pom.xmlnarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsSlotsBenchmark.java
3a97f23 to
c984b72
Compare
c0a91a7 to
5af708c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/benchmark.sh:
- Line 113: Update the BM5 benchmark selector configuration to use a lower bound
of 9 instead of 8, ensuring all four added StoreBenchmark methods are required
while preserving the existing selector.
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsStoreBenchmark.java`:
- Around line 90-115: Remove the stale “configure 3 node cluster” TODO from the
setup method in JGroupsRaftSlotsStoreBenchmark, since
JGroupsRaftClusterStoreBenchmark now covers that configuration. Leave the
existing single-node benchmark setup unchanged.
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java`:
- Around line 113-127: Update the transaction setup and cleanup around
tm.begin() in JTAStoreBase so rollback occurs only when this method successfully
started the transaction. Track that state with a local transactionStarted flag,
set it after begin succeeds, and guard both the catch-path rollback and
finally-block rollback with it, preserving suppressed-exception handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5676c249-55bc-4a74-9f89-97b5e1e0fa1b
📒 Files selected for processing (8)
.github/scripts/benchmark.shnarayana/ArjunaJTA/jta/etc/jgroups-raft.xmlnarayana/ArjunaJTA/jta/etc/jgroups.xmlnarayana/ArjunaJTA/jta/pom.xmlnarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftClusterStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsSlotsStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsStoreBenchmark.java`:
- Around line 135-144: Update testJGroupsRaftSlotsStore so RollbackException is
not counted as a successful JMH operation: add and increment JMH auxiliary
counters for committed and rolled-back executions, and publish committed
throughput separately while recording rollback failures. Do not let the
exception handler return as an unclassified successful invocation; preserve the
existing transaction invocation and Blackhole consumption for committed results.
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java`:
- Around line 120-127: Update the finally-block cleanup in JTAStoreBase so it
runs only when the transaction was started, and wrap both tm.getTransaction()
and rollback failures as best-effort cleanup. Preserve the original exception
while attaching any cleanup failure via addSuppressed, including failures
retrieving the transaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8ffea7d3-2358-4ec7-87ae-a20ba7385b4f
📒 Files selected for processing (3)
narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsSlotsStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/benchmark.sh:
- Line 113: Update the BM5 benchmark pattern to expect 11 CSV rows, accounting
for the committed and rolledBack auxiliary rows emitted by
JGroupsRaftSlotsStoreBenchmark while preserving the existing benchmark
selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cd24571f-5b77-47d5-b175-7e85520ddcfd
📒 Files selected for processing (3)
.github/scripts/benchmark.shnarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java
…uster of JGroupsRaftSlots (using SHARED_LOOPBACK)
…ounts and has headroom and ensure that the slot config for numberOfSlots is applied SlotStoreEnvironmentBean (required because the SlotStoreAdaptor no-arg constructor creates the SlotStore with SlotStoreEnvironmentBean).
…d, so at high thread counts (e.g. -t 1600) the slot pool fills faster than transactions complete. Cap slot allocation to the THREADS constant and swallow RollbackException in the benchmark method so -foe true JMH flag does not kill the entire JMH run.
JGroupsRaftSlotsStoreBenchmark: replace bh.consume(false) on RollbackException with @AuxCounters so JMH reports committed and rolledBack throughput separately instead of inflating the primary score with failed operations. benchmark.sh: bump store benchmark count to 9 for JGroupsRaftClusterStoreBenchmark."
| throw new Error(e); | ||
| } catch (Exception e) { | ||
| try { | ||
| tm.rollback(); |
There was a problem hiding this comment.
This is very interesting. If I am correctly understanding we are also able to count the rolled back transactions with the benchmark now.
There was a problem hiding this comment.
We can, it's nice isn't it. We can count pretty much any numeric type and have it reported as a separate benchmark run.
|
Thanks @mmusgrov for your PR! I believe letter E is showing results of 'JGroupsRaftSlotsStoreBenchmark.testJGroupsRaftSlotsStore:rolledBack' which is 0, but JGroupsRaftSlotsStoreBenchmark.testJGroupsRaftSlotsStore is not listed instead (which means it is not visible in the png file). I can read '"Param: allowDirtyReads"' in the run output, I noticed allowDirtyReads is not a Param, which is great, but I don't understand how that works. Do the throughputs listed set is as false or true? |
I don't understand the output, it looks like a bug in the pre-existing io.narayana.perf.product.ProductComparison.java and io.narayana.perf.product.ReportGenerator.java logic - the store comparison bencharks are getting conflated with the JTA product comparisons when they are completely independent from each other. I will investigate and get back to you.
This was a difficult one to answer since multiple factors could be at play and I'm not a performance expert: hardware differences, fsync performance is critical (eg on tmpfs it's a no-op which obviously is unsuitable for transaction logs), error margins on the JMH measurements etc. but I'll provide my analysis with the caveat that it's by no means definitive: Even though slot store can process reads and writes concurrently Raft serializes through a single thread. Therefore
The CI benchmarks seem to corroborate this rationale: throughput increases at a faster rate as more threads are added with JGroupsSlotsStore - the crossover happening somewhere between 24 and 240 threads On my laptop both stores scale similarly with thread count. At 1 and 240 threads they're roughly equal and at 24 threads ReplCache has the edge. The error margins are large (especially ReplCache's ±36,010 at 240 threads), so the differences aren't statistically significant. A google search reports that JGroups Raft supports [quote] "batching multiple pending proposals and entries into fewer network/log append operations, relying on underlying transport optimization and the architectural message bundling/batching capabilities provided by the core JGroups framework". So this will account for some of the performance benefits seen by Raft which might account for some of the higher than expected throughput. Some other things to consider (though I'm not sure how to relate them to the varying benchmark results on CI and my laptop) are:
The JMH The reason I did this is I wanted to characterise the overhead of routing reads and writes through the Raft leader: readFromFollower + allowDirtyReads=true is ~800x faster on my laptop with "-t 4 -f 1 -wi 3 -i 5 -r 1" than the other three combinations which makes sense because "reads are served from each node’s local in-memory state machine" so readers avoid the Raft protocol overhead of getting the value from the leader. readFromLeader + allowDirtyReads=true|false makes little difference because the leader can consult the local in-memory state machine. The kind of layout of the JMH results should include a column called (allowDirtyReads), on my laptop, for example: I will say that benchmarking on CI and laptops has always been fraught with issues. |
Thanks @mmusgrov for clarifying! This was surely useful. The PR looks good and it would be great to improve the performance output in the future (especially the png file which is not so clear IMO). Having the allowDirtyReads would be optimal or alternatively having methods like writeToLeaderWithallowDirtyReadsFalse can be good too. [UPDATE] For 'allowDirtyReads'=true I would expect to see similar performance for 'write' and improved performance for 'readFromFollower' executions as the reading is done in the local cache. |
marcosgopen
left a comment
marcosgopen
left a comment
There was a problem hiding this comment.
Thanks @mmusgrov . I am approving but I mention I am not an expert on the jgroup configuration file so I suppose those file have the default/recommended configuration for this performance testing. Additionally, as I mentioned above, we have space for improvements in the performance output files (readability) in the future.
…the counters for committed and rolledBack and not summing to the total number of transactions because the metric includes the warmup time whereas the primary benchmark only reports during the measurement window.
…coded Comparison.length() and skip JMH sub-metrics (:committed, :rolledBack) that were overwriting aggregate results
@marcosgopen I raised an issue for this problem |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftClusterStoreBenchmark.java`:
- Around line 118-160: Replace the fixed 500 ms sleep after the leader
pre-population loop with an explicit wait that verifies followerSlots has
applied all PREPOPULATE_SLOTS entries before benchmarking begins. Use the
existing JGroupsRaftSlots state or application/index inspection APIs available
in the class, and fail or time out clearly if follower replication does not
complete; keep readFromFollower() from starting until the follower is caught up.
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java`:
- Around line 109-115: In the catch block where tm.rollback() is called
unconditionally, add a guard condition to check that tm.getTransaction() is
non-null before attempting the rollback. When tm.commit() throws
RollbackException, the transaction manager removes the thread association in a
finally block before propagating the exception, so calling tm.rollback()
unconditionally triggers an IllegalStateException. Only call tm.rollback() if
the transaction is still associated with the current thread, which preserves
cleanup for cases where the transaction remains active due to enlistment
failures.
In
`@narayana/ArjunaJTA/jta/tests/classes/io/narayana/perf/product/ReportGenerator.java`:
- Around line 137-138: The key used in row.addColumn is constructed from only
the class name extracted from prod, which causes different benchmark methods and
parameter values to collide under the same key, allowing later rows to overwrite
earlier ones. Augment the key passed to row.addColumn to include the benchmark
method name and any relevant parameter values (such as allowDirtyReads) along
with the class name, ensuring each unique combination of class, method, and
parameters produces a distinct key that preserves all benchmark series in the
report.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ff8c15c-c43a-4d44-b788-423e22945212
📒 Files selected for processing (9)
.github/scripts/benchmark.shnarayana/ArjunaJTA/jta/etc/jgroups-raft.xmlnarayana/ArjunaJTA/jta/etc/jgroups.xmlnarayana/ArjunaJTA/jta/pom.xmlnarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftClusterStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsSlotsStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.javanarayana/ArjunaJTA/jta/tests/classes/io/narayana/perf/product/ReportGenerator.java
… finished the transaction.
… benchmark starts.
|
The successful performance run was triggered from the main PR. |
3 participants
https://redhat.atlassian.net/browse/JBTM-4038
NARAYANA_REPO=mmusgrov
NARAYANA_BRANCH=JBTM-4038
NY_BRANCH=JBTM-4038
THREAD_COUNTS=1 24 240
COMPARE_STORES=y
COMPARE_IMPLEMENTATIONS=n
COMPARE_TRANSPORTS=n
COMPARE_JOURNAL_PARAMETERS=n
JDK25