GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — avoid wrapping materialized fieldValueObject in a CompletableFuture by samuelAndalon · Pull Request #3943 · graphql-java/graphql-java

Conversation

@samuelAndalon

@samuelAndalon samuelAndalon commented Apr 29, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

While updating graphql-kotlin to graphql-java 23 found that completeField is still wrapping FieldValueInfo.fieldValueObject in a CompletableFuture for objects already materialized / in memory

this might be considered a follow up of the Completable Future wrapping change set that came in graphql-java 22
https://github.com/graphql-java/graphql-java/releases/tag/v22.0

I don't think this is a breaking change.

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
Samuel Vazquez
avoid wrapping materialized fieldValueObject in a completable future
2e25aa8
@samuelAndalon samuelAndalon changed the title avoid wrapping materialized fieldValueObject in a completable future avoid wrapping materialized fieldValueObject in a CompletableFuture Apr 29, 2025
samuelAndalon
samuelAndalon commented Apr 29, 2025
View reviewed changes
Comment thread src/main/java/graphql/execution/ExecutionStrategy.java
CompletableFuture<Object> executionResultFuture = fieldValueInfo.getFieldValueFuture();
ctxCompleteField.onDispatched();
executionResultFuture.whenComplete(ctxCompleteField::onCompleted);
if (fieldValueInfo.isFutureValue()) {

@samuelAndalon samuelAndalon Apr 29, 2025

Copy link
Copy Markdown
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.

I could've done this as well but it has the unchecked cast already handled internally by FieldValueInfo

 Object executionResult = fieldValueInfo.getFieldValueObject();
 ctxCompleteField.onDispatched();
  if (executionResult instanceof CompletableFuture) {
   @SuppressWarnings("unchecked")
    CompletableFuture<Object> executionResultFuture = (CompletableFuture<Object>) executionResult;
    executionResultFuture.whenComplete(ctxCompleteField::onCompleted);
 } else {
     ctxCompleteField.onCompleted(executionResult, null);
 }

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Samjin
Samjin approved these changes Apr 29, 2025
View reviewed changes
bbakerman
bbakerman approved these changes Apr 29, 2025
View reviewed changes

@bbakerman bbakerman left a comment

Copy link
Copy Markdown
Member

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.

Nice catch - I am embarrassed I missed this in the great "materialised migration" effort

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
samuelAndalon reacted with thumbs up emoji dfa1 reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction
@bbakerman bbakerman added the performance work that is primarily targeted as performance improvements label Apr 29, 2025
@samuelAndalon

samuelAndalon commented Apr 29, 2025

Copy link
Copy Markdown
Author

all good! thank you, now I am curious the memory and CPU improvement this change will bring
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bbakerman bbakerman added this to the 23.1 non-breaking changes and bugfixes milestone Apr 30, 2025
@bbakerman
bbakerman merged commit ea9104d into graphql-java:master Apr 30, 2025
@samuelAndalon
samuelAndalon deleted the avoid-wrapping-fvi-completable-future branch April 30, 2025 16:34
@samuelAndalon

samuelAndalon commented Apr 30, 2025

Copy link
Copy Markdown
Author

@bbakerman I wonder if this can be cherry picked to version 22.x ? I can prepare the PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@samuelAndalon samuelAndalon mentioned this pull request Apr 30, 2025
avoid wrapping materialized fieldValueObject in a completable future #3944
Merged
@dondonz dondonz modified the milestones: 23.x non-breaking changes and bugfixes, 23.2, 24.0 May 11, 2025
dondonz added a commit that referenced this pull request May 13, 2025
@dondonz
Backport of #3943
3ae003d
@dondonz dondonz mentioned this pull request May 13, 2025
24.x Backport of avoid wrapping materialised value #3962
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbakerman bbakerman bbakerman approved these changes
+1 more reviewer
@Samjin Samjin Samjin approved these changes
Reviewers whose approvals may not affect merge requirements

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.

4 participants

@samuelAndalon @bbakerman @Samjin @dondonz
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.