GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Stop creating NonNullableFieldValidator every for every object or list field by bbakerman · Pull Request #3929 · graphql-java/graphql-java

Conversation

bbakerman commented Apr 24, 2025

Copy link

Copy Markdown

Member

Today we create a NonNullableFieldValidator for every list item and every object we encounter. We do this because the constructor took a ExecutionStepInfo which IF there was a problem was used say what field was in error

BUT we dont need this. It is passed into the validate method via the ExecutionStrategyParameters anyway

So we can create 1 at execution time and use it through out

This save nemory allocation for cases where you have lots of object fields and / or lost of list fields.

This is NOT a breaking change - NonNullableFieldValidator is a internal class and this only changes the constructor shape

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dfa1 reacted with heart emoji

All reactions

❤️ 1 reaction


          Stop creating NonNullableFieldValidator every for every object or lis…

d472058

…t field

bbakerman added this to the 23.1 non-breaking changes and bugfixes milestone

Apr 24, 2025

bbakerman commented

Apr 24, 2025

View reviewed changes

Comment thread

src/main/java/graphql/execution/Execution.java

		ResultPath path = ResultPath.rootPath();
		ExecutionStepInfo executionStepInfo = newExecutionStepInfo().type(operationRootType).path(path).build();
		NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, executionStepInfo);
		NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext);

bbakerman Apr 24, 2025

Copy link

Copy Markdown

Member Author

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.

No need for the executionStepInfo here. Its passed in during validate

So this instance gets used for the entirety of the operation

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 24, 2025

View reviewed changes

Comment thread

src/main/java/graphql/execution/ExecutionStrategy.java

		instrumentationParams, executionContext.getInstrumentationState()
		));

		NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, executionStepInfo);

bbakerman Apr 24, 2025

Copy link

Copy Markdown

Member Author

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.

no need to recreate it

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 24, 2025

View reviewed changes

Comment thread

src/main/java/graphql/execution/ExecutionStrategy.java Outdated

		NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, executionStepInfo);

		ExecutionStrategyParameters newParameters = parameters.transform(builder ->
		builder.executionStepInfo(executionStepInfo)

bbakerman Apr 24, 2025

Copy link

Copy Markdown

Member Author

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.

see - executionStepInfo is in the ExecutionStrategyParameters always

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 24, 2025

View reviewed changes

Comment thread

src/main/java/graphql/execution/ExecutionStrategy.java

		ExecutionStepInfo stepInfoForListElement = executionStepInfoFactory.newExecutionStepInfoForListElement(executionStepInfo, indexedPath);

		NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, stepInfoForListElement);

bbakerman Apr 24, 2025

Copy link

Copy Markdown

Member Author

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.

No need - same reason

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 24, 2025

View reviewed changes

Comment thread

src/main/java/graphql/execution/ExecutionStrategyParameters.java

		this.fields = assertNotNull(fields, () -> "fields is null");
		this.source = source;
		this.nonNullableFieldValidator = nonNullableFieldValidator;
		this.nonNullableFieldValidator = assertNotNull(nonNullableFieldValidator, () -> "requires a NonNullValidator");;

bbakerman Apr 24, 2025

Copy link

Copy Markdown

Member Author

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.

Just to be more specific - this helped find tests that "half create" objects for testing reasons

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 24, 2025

View reviewed changes

Comment thread

src/main/java/graphql/execution/SubscriptionExecutionStrategy.java Outdated

		return parameters.transform(builder -> builder.field(firstField).path(fieldPath));
		NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext);
		return parameters.transform(builder ->
		builder.field(firstField).path(fieldPath).nonNullFieldValidator(nonNullableFieldValidator));

bbakerman Apr 24, 2025

Copy link

Copy Markdown

Member Author

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.

Subscriptions is a special case - because it starts a new ExecutionContext instance - So we do re-create it here - but only once - not per published message

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 24, 2025

View reviewed changes

Comment thread

src/test/groovy/graphql/execution/AsyncExecutionStrategyTest.groovy

		.newParameters()
		.executionStepInfo(typeInfo)
		.fields(mergedSelectionSet(['hello': mergedField([Field.newField('hello').build()]), 'hello2': mergedField([Field.newField('hello2').build()])]))
		.nonNullFieldValidator(new NonNullableFieldValidator(executionContext))

bbakerman Apr 24, 2025

Copy link

Copy Markdown

Member Author

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.

The rest of the changes are test fixups where they half create support objects with just enough brains to work

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

github-actions Bot commented Apr 24, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Test Results

314 files ±0 314 suites ±0 52s ⏱️ -2s
3 610 tests ±0 3 605 ✅ ±0 5 💤 ±0 0 ❌ ±0
3 699 runs ±0 3 694 ✅ ±0 5 💤 ±0 0 ❌ ±0

Results for commit 80f4a3b. ± Comparison against base commit 397c050.

This pull request removes 174 and adds 152 tests. Note that renamed tests count towards both.

	?
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
                a2 :  __type(name : "t1") { name }
…

graphql.DataFetcherTest ‑ get Boolean property value [fetcher: <graphql.schema.PropertyDataFetcher@7f1ef916 propertyName=booleanField function=null>, #0]
graphql.DataFetcherTest ‑ get Boolean property value [fetcher: <graphql.schema.SingletonPropertyDataFetcher@5c441290>, #1]
graphql.DataFetcherTest ‑ get Boolean property value with get [fetcher: <graphql.schema.PropertyDataFetcher@17dd671f propertyName=booleanFieldWithGet function=null>, #0]
graphql.DataFetcherTest ‑ get Boolean property value with get [fetcher: <graphql.schema.SingletonPropertyDataFetcher@5c441290>, #1]
graphql.DataFetcherTest ‑ get property value [fetcher: <graphql.schema.PropertyDataFetcher@47c40b56 propertyName=property function=null>, #0]
graphql.DataFetcherTest ‑ get property value [fetcher: <graphql.schema.SingletonPropertyDataFetcher@5c441290>, #1]
graphql.DataFetcherTest ‑ get public field value as property [fetcher: <graphql.schema.PropertyDataFetcher@9b9a327 propertyName=publicField function=null>, #0]
graphql.DataFetcherTest ‑ get public field value as property [fetcher: <graphql.schema.SingletonPropertyDataFetcher@5c441290>, #1]
graphql.ScalarsBooleanTest ‑ parseValue throws exception for invalid input <java.lang.Object@3f17bc1e>
graphql.ScalarsBooleanTest ‑ serialize throws exception for invalid input <java.lang.Object@1f9b614d>
…

♻️ 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.

bbakerman added the performance work that is primarily targeted as performance improvements label

Apr 24, 2025

bbakerman requested review from andimarek and dondonz

April 27, 2025 06:31

bbakerman mentioned this pull request

Apr 29, 2025

1% possible memory and cpu improvements #3939

Open

dondonz modified the milestones: 25.x breaking changes, 24.0

May 13, 2025

dondonz approved these changes

May 14, 2025

View reviewed changes

bbakerman added 2 commits

May 15, 2025 15:04


          Merge remote-tracking branch 'origin/master' into dont-alllocate-NonN…

…ullableFieldValidator-all-the-time

# Conflicts:
#	src/main/java/graphql/execution/ExecutionStrategy.java
#	src/main/java/graphql/execution/SubscriptionExecutionStrategy.java


          Merged in master

80f4a3b

dondonz merged commit abb2cdd into master

May 15, 2025

dondonz mentioned this pull request

May 15, 2025

Cherry pick PR 3929 reduce nonnullablefieldvalidator allocations #3974

Merged

Reviewers

dondonz

dondonz approved these changes

andimarek

Awaiting requested review from andimarek

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

performance

work that is primarily targeted as performance improvements

Projects

None yet

Milestone

24.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

bbakerman commented Apr 24, 2025

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!

github-actions Bot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Apr 24, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.