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 — Retriveing singleton property fetchers will not create DataFetcherFactoryEnvironment objects by bbakerman · Pull Request #3942 · graphql-java/graphql-java

Conversation

@bbakerman

@bbakerman bbakerman commented Apr 29, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This was always the intention here when we created #3754 to have a singleton property fetcher instance

However we didnt do the factory right and hence DataFetcherFactoryEnvironment are created when they dont need to be

This fixes on of the items found in #3939

Namely why was "graphql.schema.DataFetcherFactoryEnvironment" being 2% of objects allocated

This fixes that

re : #3941

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bbakerman
This is a performance improvement for property data fetchers to not c…
ade6100 
…reate `DataFetcherFactoryEnvironment` objects for simple property fetchers
@bbakerman bbakerman added the performance work that is primarily targeted as performance improvements label Apr 29, 2025
@bbakerman
bbakerman requested review from andimarek and dondonz April 29, 2025 13:00
bbakerman
bbakerman commented Apr 29, 2025
View reviewed changes
Comment thread src/main/java/graphql/schema/SingletonPropertyDataFetcher.java
public DataFetcher<Object> get(GraphQLFieldDefinition fieldDefinition) {
return SINGLETON_FETCHER;
}
};

@bbakerman bbakerman Apr 29, 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.

because I didnt override the default public DataFetcher<Object> get(GraphQLFieldDefinition fieldDefinition) then it returned null and hence it went to the older more heavy weight object and a DataFetcherFactoryEnvironment needed to be allocated.

This now means that no DataFetcherFactoryEnvironment objects are allocated for property fetchers

Sorry, something went wrong.

Uh oh!

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

@bbakerman bbakerman Apr 29, 2025 •
edited
Loading

Uh oh!

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

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.

I cant unit test this - its not observable - but profiling shows it to be working!

Sorry, something went wrong.

Uh oh!

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

@dondonz dondonz May 14, 2025

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 you could verify via profiling

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@github-actions

github-actions Bot commented Apr 29, 2025

Copy link
Copy Markdown
Contributor

Test Results

  312 files    312 suites   53s ⏱️
3 581 tests 3 576 ✅ 5 💤 0 ❌
3 670 runs  3 665 ✅ 5 💤 0 ❌

Results for commit ade6100.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dondonz dondonz modified the milestones: 24.x non-breaking changes and bugfixes, 25.x breaking changes, 24.0 May 11, 2025
dondonz
dondonz approved these changes May 14, 2025
View reviewed changes
@dondonz
dondonz merged commit d5b6498 into master May 14, 2025
@dondonz dondonz mentioned this pull request May 15, 2025
Cherry pick PR 3942 To add singleton to factory method #3968
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dondonz dondonz dondonz approved these changes
@andimarek 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

@bbakerman @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.