GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Per level tracking simplification and performance improvements by andimarek · Pull Request #4128 · graphql-java/graphql-java Conversation
|
Test Results 326 files 326 suites 3m 30s ⏱️ Results for commit 2eecad7. ♻️ This comment has been updated with latest results. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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 |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; | |
| } |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
| if (callStack.tryUpdateLevel(curLevel, currentState, currentState.increaseHappenedExecuteObjectCalls())) { | ||
| return; | ||
| } | ||
|
|
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
| CallStack.StateForLevel currentState = callStack.get(level); | ||
| if (callStack.tryUpdateLevel(level, currentState, currentState.increaseHappenedCompletionFinishedCount())) { | ||
| break; | ||
| } |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another infinite CAS retry loop without backoff. This could lead to high CPU usage under contention. Consider adding yield or backoff strategy.
| } | |
| } | |
| Thread.yield(); |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add assertion
…be negative effects
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.