A smidge faster unwrap non-null by bbakerman · Pull Request #4059 · graphql-java/graphql-java

bbakerman · 2025-07-25T06:27:33Z

#3939 shows that graphql.schema.GraphQLTypeUtil#unwrapNonNull is called a lot

This is a smidge faster since it does less checking of values

cpu hot spots to look into

1.5% - 1,190 ms - 7,119,736 hot spot inv. graphql.schema.GraphQLTypeUtil.unwrapNonNull

1.5% - 1,209 ms - 7,088,722 hot spot inv. graphql.execution.ExecutionStepInfo.getUnwrappedNonNullType

So graphql.execution.ExecutionStepInfo.getUnwrappedNonNullType calls graphql.schema.GraphQLTypeUtil.unwrapNonNull so we can discount it but this makes the underlying method a smidge quicker

Also we save a extra loop in the lookup because double non null wrapping cant happen and hence we can save the loop checking

github-actions · 2025-07-25T06:41:33Z

Test Results

324 files 324 suites 3m 29s ⏱️
5 006 tests 5 000 ✅ 6 💤 0 ❌
5 095 runs 5 089 ✅ 6 💤 0 ❌

Results for commit 7e0a66e.

♻️ This comment has been updated with latest results.

bbakerman · 2025-07-25T07:05:28Z

src/main/java/graphql/execution/ExecutionStepInfo.java

+     */
+    public <T extends GraphQLOutputType> T getUnwrappedNonNullTypeAs() {
+        return GraphQLTypeUtil.unwrapNonNullAs(this.type);
+    }


makes the consuming code easier to read

bbakerman · 2025-07-25T07:07:55Z

src/main/java/graphql/schema/GraphQLNonNull.java

-        assertTrue(!GraphQLTypeUtil.isNonNull(wrappedType),
-                "A non null type cannot wrap an existing non null type '%s'", GraphQLTypeUtil.simplePrint(wrappedType));
+        assertTrue(!GraphQLTypeUtil.isNonNull(wrappedType), () ->
+                String.format("A non null type cannot wrap an existing non null type '%s'", GraphQLTypeUtil.simplePrint(wrappedType)));


technically more efficient

dfa1 · 2025-07-25T08:03:14Z

src/main/java/graphql/schema/GraphQLTypeUtil.java

    public static GraphQLType unwrapNonNull(GraphQLType type) {
-        while (isNonNull(type)) {
-            type = unwrapOne(type);
+        // its illegal to have a type that is a non-null wrapping a non-null type


what if the input type is non-null list of non-null type? it should be handled here? IIRC I'm using this method somewhere in one project

GraphqlNonNull has code to prevent it being double wrapped.

So

field( arg : String!!)

aka

nonNull(nonNull(GraphqlString)) is actually illegal and wont ever be allowed

Ok that is good but what about a type like

ids: [ID!]!?

I suspect that the while loop was there to handle that

In the case of [ID!]! it's not non-null wrapping a non-null, it would be non-null wrapping a list, wrapping a non-null

Ok but I think this could be a breaking change for some projects (and this class is not marked as @internal IIRC)

Wrapping a non-null with another non-null is not allowed as per the specification: in the grammar it can only wrap a named or list type, not another non-null type. https://spec.graphql.org/draft/#sec-Type-References

Is there a situation where you would have had a non-null wrapping another non-null in your experience?

I thought about the breaking change aspect and its not there. We already had assertion code to prevent ids : ID!! as a type so the code was overly defensive and I suspect written without the understanding that double non nulls is actually illegal.

So there is no breaking change here. Just a more efficient unwrapping of a single non null layer.

the problem I see is that the previous code was unwrapping all the wrapper types (because unwrapOne/getWrappedType work for List too):
so, previously, a client of this method could call it with [ID!]! to get ID. Is there a unit test for this behavior?

Of course, I could be wrong ... I'm pretty sure my code could be broken by this change but I don't have my laptop with me now.

The problem I see is that the previous code was unwrapping all the wrapper types (because unwrapOne/getWrappedType work for List too):

Its ok - itt didnt work like that. It only every unwrapped a non null . The code was

public static GraphQLType unwrapNonNull(GraphQLType type) { while (isNonNull(type)) { type = unwrapOne(type); } public static GraphQLType unwrapOne(GraphQLType type) { if (isNonNull(type)) { return ((GraphQLNonNull) type).getWrappedType(); } else if (isList(type)) { return ((GraphQLList) type).getWrappedType(); } return type; }

So it would check if its non null and stop once it reached a type that is not non null. The reason its faster is that unwrapOne does another isNonNull instanceof check inside itself. So this avoids that.

Its not a major saving but its some saving. Thats why I said its a smidge faster

smidgen
/ˈsmɪdʒɪn/
nouninformal
noun: smidgen; plural noun: smidgens; noun: smidgin; plural noun: smidgins; noun: smidgeon; plural noun: smidgeons
a small amount of something.
"add a smidgen of cayenne"

looks good then, thanks for the explanation!

dondonz · 2025-07-27T20:35:19Z

src/main/java/graphql/schema/GraphQLNonNull.java

-        assertTrue(!GraphQLTypeUtil.isNonNull(wrappedType),
-                "A non null type cannot wrap an existing non null type '%s'", GraphQLTypeUtil.simplePrint(wrappedType));
+        assertTrue(!GraphQLTypeUtil.isNonNull(wrappedType), () ->
+                "A non null type cannot wrap an existing non null type");


Did you mean to remove the name of the type in this error message?

Yes I did - why?

because a static value that takes in no "enclosed" variables allocates no memory - the Supplier becomes a global singleton compiled into place.

but if we "enclose" a variable then it allocates the Supplier on every call.

So in the 0.0001% case where we have an error, yes the error message is better BUT we pay a memory price on the 99.9999% of the time where there is no error.

So yes its a less informative error message, but its also better at memory usage!

A smidge faster unwrap non-null

51db09b

bbakerman added this to the 25.x breaking changes milestone Jul 25, 2025

A smidge faster unwrap non-null - updated to only do 1 check

4dc1f34

bbakerman commented Jul 25, 2025

View reviewed changes

A smidge faster unwrap non-null - asssert tweak

7e0a66e

dfa1 reviewed Jul 25, 2025

View reviewed changes

dondonz reviewed Jul 27, 2025

View reviewed changes

dondonz approved these changes Jul 27, 2025

View reviewed changes

bbakerman merged commit 80f35b3 into master Jul 30, 2025
2 checks passed

Conversation

bbakerman commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfa1 Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbakerman Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbakerman commented Jul 25, 2025 •

edited

Loading

github-actions bot commented Jul 25, 2025 •

edited

Loading

dfa1 Jul 29, 2025 •

edited

Loading

bbakerman Jul 29, 2025 •

edited

Loading