Skip to content

Per level tracking simplification and performance improvements #4128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andimarek merged 13 commits into from
Sep 24, 2025
Merged

Per level tracking simplification and performance improvements #4128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9566326
nearly works
andimarek Sep 23, 2025
07b1600
works except dataloader in defer
andimarek Sep 23, 2025
56a39df
working with simpler mode except batching, defer, subscription
andimarek Sep 24, 2025
0436e5d
subscription works
andimarek Sep 24, 2025
b25724b
fix defer support
andimarek Sep 24, 2025
117ebe1
cleanup
andimarek Sep 24, 2025
a5d29a6
cleanup
andimarek Sep 24, 2025
2686c32
no per level tracking anymore
andimarek Sep 24, 2025
a67e56b
cleanup
andimarek Sep 24, 2025
2da8fef
no synchronized/locking anymore
andimarek Sep 24, 2025
009fcb8
cleanup and comment
andimarek Sep 24, 2025
ab06e30
add Thread.onSpinWait to indicate active waiting
andimarek Sep 24, 2025
2eecad7
remove Thread.onSpinWait again as not clear this is needed or has may…
andimarek Sep 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
no per level tracking anymore
  • Loading branch information
@andimarek
andimarek committed Sep 24, 2025
commit 2686c32e64cbd240b63ef7e7e345e19daec9e654
36 changes: 18 additions & 18 deletions src/jmh/java/performance/DataLoaderPerformance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import graphql.ExecutionInput;
import graphql.ExecutionResult;
import graphql.GraphQL;
import graphql.execution.instrumentation.dataloader.DataLoaderDispatchingContextKeys;
import graphql.schema.DataFetcher;
import graphql.schema.GraphQLSchema;
import graphql.schema.idl.RuntimeWiring;
Expand All @@ -15,28 +14,16 @@
import org.dataloader.DataLoader;
import org.dataloader.DataLoaderFactory;
import org.dataloader.DataLoaderRegistry;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;

import java.util.List;
import java.util.Map;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

@State(Scope.Benchmark)
@Warmup(iterations = 2, time = 5)
@Measurement(iterations = 3)
@Fork(2)
public class DataLoaderPerformance {

static Owner o1 = new Owner("O-1", "Andi", List.of("P-1", "P-2", "P-3"));
Expand Down Expand Up @@ -573,9 +560,6 @@ public void setup() {

}

@Benchmark
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
public void executeRequestWithDataLoaders(MyState myState, Blackhole blackhole) {
DataLoader ownerDL = DataLoaderFactory.newDataLoader(ownerBatchLoader);
DataLoader petDL = DataLoaderFactory.newDataLoader(petBatchLoader);
Expand All @@ -587,14 +571,30 @@ public void executeRequestWithDataLoaders(MyState myState, Blackhole blackhole)
.dataLoaderRegistry(registry)
// .profileExecution(true)
.build();
executionInput.getGraphQLContext().put(DataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_CHAINING, true);
// executionInput.getGraphQLContext().put(DataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_CHAINING, true);
ExecutionResult execute = myState.graphQL.execute(executionInput);
// ProfilerResult profilerResult = executionInput.getGraphQLContext().get(ProfilerResult.PROFILER_CONTEXT_KEY);
// System.out.println(profilerResult.shortSummaryMap());
// System.out.println("execute: " + execute);
Assert.assertTrue(execute.isDataPresent());
Assert.assertTrue(execute.getErrors().isEmpty());
blackhole.consume(execute);
}

public static void main(String[] args) {
DataLoaderPerformance dataLoaderPerformance = new DataLoaderPerformance();
MyState myState = new MyState();
myState.setup();
Blackhole blackhole = new Blackhole("Today's password is swordfish. I understand instantiating Blackholes directly is dangerous.");
for (int i = 0; i < 1; i++) {
dataLoaderPerformance.executeRequestWithDataLoaders(myState, blackhole);
}
// System.out.println(PerLevelDataLoaderDispatchStrategy.fieldFetchedCount);
// System.out.println(PerLevelDataLoaderDispatchStrategy.onCompletionFinishedCount);
// System.out.println(PerLevelDataLoaderDispatchStrategy.isReadyCounter);
// System.out.println(Duration.ofNanos(PerLevelDataLoaderDispatchStrategy.isReadyCounterNS.get()).toMillis());


}


}
48 changes: 31 additions & 17 deletions ...java/graphql/execution/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.jspecify.annotations.Nullable;

import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -139,7 +140,7 @@ private static class CallStack {
private final LevelMap happenedCompletionFinishedCountPerLevel = new LevelMap();
private final LevelMap happenedExecuteObjectCallsPerLevel = new LevelMap();

private final Set<Integer> dispatchedLevels = ConcurrentHashMap.newKeySet();
private final Set<Integer> dispatchedLevels = new LinkedHashSet<>();


public ChainedDLStack chainedDLStack = new ChainedDLStack();
Expand Down Expand Up @@ -228,9 +229,11 @@ public void executeObject(ExecutionContext executionContext, ExecutionStrategyPa
public void executeObjectOnFieldValuesException(Throwable t, ExecutionStrategyParameters parameters) {
CallStack callStack = getCallStack(parameters);
int curLevel = parameters.getPath().getLevel();
// System.out.println("completion finished for level " + curLevel);
onCompletionFinished(curLevel, callStack);
}


private void onCompletionFinished(int level, CallStack callStack) {
synchronized (callStack) {
callStack.happenedCompletionFinishedCountPerLevel.increment(level, 1);
Expand All @@ -241,20 +244,23 @@ private void onCompletionFinished(int level, CallStack callStack) {
boolean levelReady;
synchronized (callStack) {
if (callStack.dispatchedLevels.contains(currentLevel)) {
// System.out.println("failed because already dispatched");
break;
}
levelReady = markLevelAsDispatchedIfReady(currentLevel, callStack);
}
if (levelReady) {
dispatch(currentLevel, callStack);
} else {
// System.out.println("failed because level not ready");
break;
}
currentLevel++;
}

}


@Override
public void fieldFetched(ExecutionContext executionContext,
ExecutionStrategyParameters executionStrategyParameters,
Expand All @@ -263,12 +269,20 @@ public void fieldFetched(ExecutionContext executionContext,
Supplier<DataFetchingEnvironment> dataFetchingEnvironment) {
CallStack callStack = getCallStack(executionStrategyParameters);
int level = executionStrategyParameters.getPath().getLevel();
boolean dispatchNeeded;
synchronized (callStack) {
callStack.happenedFetchCountPerLevel.increment(level, 1);
dispatchNeeded = markLevelAsDispatchedIfReady(level, callStack);
boolean dispatchNeeded = false;
// System.out.println("field fetched for level " + level + " path: " + executionStrategyParameters.getPath());
AlternativeCallContext deferredCallContext = executionStrategyParameters.getDeferredCallContext();
if (level == 1 || (deferredCallContext != null && level == deferredCallContext.getStartLevel())) {
synchronized (callStack) {
callStack.happenedFetchCountPerLevel.increment(level, 1);
dispatchNeeded = callStack.expectedFetchCountPerLevel.get(level) == callStack.happenedFetchCountPerLevel.get(level);
if (dispatchNeeded) {
callStack.dispatchedLevels.add(level);
}
}
}
if (dispatchNeeded) {
// System.out.println("Success field fetch");
dispatch(level, callStack);
}

Expand Down Expand Up @@ -361,27 +375,25 @@ private boolean markLevelAsDispatchedIfReady(int level, CallStack callStack) {

private boolean isLevelReady(int level, CallStack callStack) {
// a level with zero expectations can't be ready
int expectedFetchCount = callStack.expectedFetchCountPerLevel.get(level);
if (expectedFetchCount == 0) {
return false;
}
// int expectedFetchCount = callStack.expectedFetchCountPerLevel.get(level);
// if (expectedFetchCount == 0) {
// return false;
// }

if (expectedFetchCount != callStack.happenedFetchCountPerLevel.get(level)) {
return false;
}
if (level == 1) {
// for the root fields we just expect that they were all fetched
return true;
}
// if (expectedFetchCount != callStack.happenedFetchCountPerLevel.get(level)) {
// return false;
// }

// 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

Copy link

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.

Suggested change
// 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;
}

Copilot uses AI. Check for mistakes.
int happenedExecuteObjectCalls = callStack.happenedExecuteObjectCallsPerLevel.get(level - 2);
return callStack.dispatchedLevels.contains(level - 1) &&
callStack.happenedExecuteObjectCallsPerLevel.get(level - 2) == callStack.happenedCompletionFinishedCountPerLevel.get(level - 2);
happenedExecuteObjectCalls > 0 && happenedExecuteObjectCalls == callStack.happenedCompletionFinishedCountPerLevel.get(level - 2);

}

void dispatch(int level, CallStack callStack) {
// System.out.println("dispatching " + level);
if (!enableDataLoaderChaining) {
profiler.oldStrategyDispatchingAll(level);
DataLoaderRegistry dataLoaderRegistry = executionContext.getDataLoaderRegistry();
Expand All @@ -392,9 +404,11 @@ void dispatch(int level, CallStack callStack) {
}

private void dispatchAll(DataLoaderRegistry dataLoaderRegistry, int level) {
// System.out.println("dispatch level " + level);
for (DataLoader<?, ?> dataLoader : dataLoaderRegistry.getDataLoaders()) {
dataLoader.dispatch().whenComplete((objects, throwable) -> {
if (objects != null && objects.size() > 0) {
// System.out.println("dispatching " + objects.size() + " objects for level " + level);
Assert.assertNotNull(dataLoader.getName());
profiler.batchLoadedOldStrategy(dataLoader.getName(), level, objects.size());
}
Expand Down