Skip to content
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.

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

Reduce calculation for fragments in ExecutableNormalizedOperation #2911

Merged
merged 4 commits into from Aug 11, 2022

Conversation

dondonz
Copy link
Member

@dondonz dondonz commented Aug 4, 2022

After parsing and validating an incoming GraphQL operation, the engine creates a normalized version of that operation, the ExecutableNormalizedOperation.

For fragments, there is a little more work involved to determine the possible types. Inside narrowDownPossibleObjects, there is a set intersection between the current set of possible objects and the set of types from the resolved type condition.

This set intersection can be expensive for very large queries with hundreds of fragments. This PR includes one such example.

In almost every case in the example query, one of the sets being compared only has one member. For example, the resolved types set often has only 1 member, because an inline fragment on a concrete type has only one possible type. It would be equivalent, and much cheaper, to calculate via Set contains rather than intersection.

And I've added a more general tweak to always pass the smallest set first to Guava's Sets.intersection. The implementation is faster when the first set is smaller.

Benchmark results

Baseline

Benchmark                                 Mode  Cnt    Score    Error  Units
NQExtraLargeBenchmark.benchMarkThroughput  thrpt    9  274.744 ± 14.909  ops/s
NQExtraLargeBenchmark.benchMarkAvgTime      avgt    9    3.658 ±  0.310  ms/op

Optimised

Benchmark                                 Mode  Cnt    Score    Error  Units
NQExtraLargeBenchmark.benchMarkThroughput  thrpt    9  333.346 ± 25.986  ops/s
NQExtraLargeBenchmark.benchMarkAvgTime      avgt    9    2.905 ±  0.124  ms/op

Note: For throughput, a higher number is better. For average time, a lower number is better.

return resolvedTypeCondition;
} else if (currentOnes.size() == 1 && resolvedTypeCondition.contains(currentOnes.iterator().next())) {
return currentOnes;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java shits me on how access to a first member is that god awful collection.iterator().next()

But here we are!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps create a utitlity method in graphql.util.FpKit

FpKit. intersection( that does this optimisation for same type sets

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea I'll stick it in FpKit instead

@dondonz dondonz changed the title Reduce calculation for inline fragments in ExecutableNormalizedOperation Reduce calculation for fragments in ExecutableNormalizedOperation Aug 5, 2022
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests for the new intersection method

return Sets.intersection(set1, set2);
}
return Sets.intersection(set2, set1);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a test for this. I know its simple. So lock it in with a test

@bbakerman bbakerman added this to the 19.1 milestone Aug 11, 2022
@dondonz dondonz merged commit 9479cc0 into master Aug 11, 2022
@dondonz dondonz deleted the benchmark_enf branch August 11, 2022 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants