Skip to content

This removes the FetchedValue wrapping by default #3924

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jul 10, 2025
Merged

This removes the FetchedValue wrapping by default #3924

merged 11 commits into from
Jul 10, 2025

Conversation

bbakerman
Copy link
Member

related to #3922

This removes FetchedValue wrapping of every value we get back from a DataFetcher

This only wraps it in a FetchedValue if the DF returned the complex graphql.execution.DataFetcherResult.

The consuming code now needs to tests if its a FetchedValue to get its underlying value or to use it direct

@bbakerman bbakerman added the breaking change requires a new major version to be relased label Apr 22, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 22, 2025 •
edited
Loading

Test Results

  323 files    323 suites   3m 2s ⏱️
4 932 tests 4 922 ✅ 10 💤 0 ❌
5 021 runs  5 011 ✅ 10 💤 0 ❌

Results for commit 095ba62.

♻️ This comment has been updated with latest results.

@bbakerman bbakerman added this to the 24.0 milestone Apr 22, 2025
@bbakerman
Copy link
Member Author

Manually run JMH tests


This branch

Benchmark                                                                           Mode  Cnt            Score            Error   Units
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput                     thrpt    4            0.247 ±          0.038   ops/s # better
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput:gc.alloc.rate       thrpt    4         4076.203 ±        258.101  MB/sec # better
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput:gc.alloc.rate.norm  thrpt    4  17320751492.000 ± 2385254881.326    B/op # slightly better
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput:gc.count            thrpt    4           69.000                   counts # same
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput:gc.time             thrpt    4        11174.000                       ms # better
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime                         avgt    4            4.228 ±          1.047    s/op # worse
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime:gc.alloc.rate           avgt    4         4019.164 ±        992.876  MB/sec # better
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime:gc.alloc.rate.norm      avgt    4  17800984633.333 ±    7488957.877    B/op # better
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime:gc.count                avgt    4           80.000                   counts # worse
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime:gc.time                 avgt    4        11929.000                       ms # worse


master


Benchmark                                                                           Mode  Cnt            Score            Error   Units
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput                     thrpt    4            0.260 ±          0.049   ops/s
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput:gc.alloc.rate       thrpt    4         4382.516 ±        694.460  MB/sec
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput:gc.alloc.rate.norm  thrpt    4  17680353848.000 ± 1492524470.152    B/op
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput:gc.count            thrpt    4           72.000                   counts
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesThroughput:gc.time             thrpt    4         8996.000                       ms
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime                         avgt    4            3.904 ±          1.378    s/op
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime:gc.alloc.rate           avgt    4         4439.397 ±       1687.989  MB/sec
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime:gc.alloc.rate.norm      avgt    4  18133700842.667 ± 2305552942.125    B/op
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime:gc.count                avgt    4           70.000                   counts
LargeInMemoryQueryPerformance.benchMarkSimpleQueriesAvgTime:gc.time                 avgt    4         9392.000                       ms

It definitely helped in using less memory. Interestingly the gc.time went up - not sure how to read that

@bbakerman bbakerman mentioned this pull request Apr 29, 2025
Open
@bbakerman bbakerman added the performance work that is primarily targeted as performance improvements label May 13, 2025
@bbakerman bbakerman requested a review from andimarek July 7, 2025 07:12
bbakerman
bbakerman commented Jul 8, 2025
View reviewed changes
...ava/graphql/execution/instrumentation/parameters/InstrumentationFieldCompleteParameters.java
*
* @return the object was fetched, ready to be completed as a value.
*/
public Object getFetchedObject() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the hard breaking change - the new method means they HAVE to handle it should they be using the old method

bbakerman
bbakerman commented Jul 8, 2025
View reviewed changes
src/test/groovy/graphql/execution/ExecutionStrategyTest.groovy
if (parameters.fetchedValue instanceof FetchedValue) {
FetchedValue value = (FetchedValue) parameters.fetchedValue
if (parameters.getFetchedObject() instanceof FetchedValue) {
FetchedValue value = (FetchedValue) parameters.getFetchedObject()
Copy link
Member Author

Choose a reason for hiding this comment

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

example of hard break!

@dondonz dondonz merged commit ecd84a1 into master Jul 10, 2025
2 checks passed
@dondonz dondonz deleted the remove-fetch-value-wrapping branch July 10, 2025 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andimarek andimarek Awaiting requested review from andimarek

Assignees
No one assigned
Labels
breaking change requires a new major version to be relased performance work that is primarily targeted as performance improvements
Projects
None yet
Milestone
25.x breaking changes
Development

Successfully merging this pull request may close these issues.
2 participants
@bbakerman @dondonz