Per level tracking simplification and performance improvements#4128
Per level tracking simplification and performance improvements#4128
Conversation
Test Results 326 files 326 suites 3m 30s ⏱️ Results for commit 2eecad7. ♻️ This comment has been updated with latest results. |
andimarek
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the PerLevelDataLoaderDispatchStrategy to simplify level tracking and improve performance by replacing synchronized blocks with lock-free compare-and-swap (CAS) operations. The refactoring reduces the complexity of tracking mechanisms and introduces atomic operations for thread safety.
- Simplified per-level tracking by consolidating multiple tracking maps into a single state object
- Replaced synchronized blocks with CAS-based atomic operations for better performance
- Restructured subscription execution flow with separate callback methods
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| PerLevelDataLoaderDispatchStrategy.java | Core refactoring with new StateForLevel class and CAS-based updates |
| LevelMap.java | Updated toString format with additional brackets |
| DataLoaderDispatchStrategy.java | Split subscription callback into separate methods |
| SubscriptionExecutionStrategy.java | Updated to use new subscription callback methods |
| ExecutionStrategy.java | Reordered executeObject call timing |
| LevelMapTest.groovy | Updated test assertions for new toString format |
| DataLoaderPerformance.java | Removed benchmark annotations and added main method for testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return true; | ||
| private boolean isLevelReady(int level, CallStack callStack) { | ||
| // we expect that parent has been dispatched and that all parents fields are completed | ||
| // all parent fields completed means all parent parent on completions finished calls must have happened |
There was a problem hiding this comment.
This line can cause an IndexOutOfBoundsException when level is less than 2. The method isLevelReady should validate that level >= 2 before accessing level - 2.
| // all parent fields completed means all parent parent on completions finished calls must have happened | |
| // all parent fields completed means all parent parent on completions finished calls must have happened | |
| if (level < 2) { | |
| return false; | |
| } |
| if (callStack.tryUpdateLevel(curLevel, currentState, currentState.increaseHappenedExecuteObjectCalls())) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This infinite loop without backoff or yield could cause excessive CPU usage under high contention. Consider adding a short pause or using exponential backoff between CAS retry attempts.
| // Avoid excessive CPU usage under contention | |
| Thread.onSpinWait(); |
| CallStack.StateForLevel currentState = callStack.get(level); | ||
| if (callStack.tryUpdateLevel(level, currentState, currentState.increaseHappenedCompletionFinishedCount())) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Another infinite CAS retry loop without backoff. This could lead to high CPU usage under contention. Consider adding yield or backoff strategy.
| } | |
| } | |
| Thread.yield(); |
Add assertion
…be negative effects
1 participant
This PR fundamentally refactors the
PerLevelDataLoaderDispatchStrategyfirst it simplifies a lot what is actually tracked and then it get rids of synchronized/locks and uses a CAS mechanism instead.
First Pr that implemented a lock free chained data loader tracking: #4123