Skip to content

Comments

Optimize DataFetchingSelectionSet.getImmediateFields() to avoid traversing descendants#3855

Merged
bbakerman merged 2 commits intographql-java:masterfrom
timward60:timward/data-fetching-selection-set-immediate-fields
Mar 20, 2025
Merged

Optimize DataFetchingSelectionSet.getImmediateFields() to avoid traversing descendants#3855
bbakerman merged 2 commits intographql-java:masterfrom
timward60:timward/data-fetching-selection-set-immediate-fields

Conversation

@timward60
Copy link
Contributor

@timward60 timward60 commented Mar 18, 2025 •
edited
Loading

Addresses #3854.

Description

Computes immediate fields only for getImmediateFields() to avoid cost of traversing the entire sub-tree to build glob-patterns for descendant fields.

Benchmark

Before:

Benchmark                                                          Mode  Cnt   Score   Error   Units
DFSelectionSetPerformance.benchMarkThroughput                     thrpt    9  10.424 ± 0.598  ops/ms
DFSelectionSetPerformance.benchMarkThroughput_getImmediateFields  thrpt    9  21.992 ± 0.224  ops/ms
DFSelectionSetPerformance.benchMarkAvgTime                         avgt    9   0.096 ± 0.005   ms/op
DFSelectionSetPerformance.benchMarkAvgTime_getImmediateFields      avgt    9   0.044 ± 0.001   ms/op

After:

Benchmark                                                          Mode  Cnt     Score    Error   Units
DFSelectionSetPerformance.benchMarkThroughput                     thrpt    9    10.641 ±  0.118  ops/ms
DFSelectionSetPerformance.benchMarkThroughput_getImmediateFields  thrpt    9  2667.783 ± 73.924  ops/ms
DFSelectionSetPerformance.benchMarkAvgTime                         avgt    9     0.094 ±  0.003   ms/op
DFSelectionSetPerformance.benchMarkAvgTime_getImmediateFields      avgt    9    ≈ 10⁻³            ms/op

@andimarek
Copy link
Member

andimarek commented Mar 18, 2025

Hey @timward60 ... thanks a lot for the PR and even more for the Benchmark.

I have a request:

We just introduced automated performance tests, which enables us to track the performance much better over time as it is executed in a stable reproducible environment.

Can you create another PR which moves the DFSelectionSetBenchmark into the performance package and renames into DFSelectionSetPerformance?

We can merge this PR first and it gives us a clear baseline for the improvements. Thanks a lot

@timward60
Copy link
Contributor Author

timward60 commented Mar 18, 2025

Hey @timward60 ... thanks a lot for the PR and even more for the Benchmark.

I have a request:

We just introduced automated performance tests, which enables us to track the performance much better over time as it is executed in a stable reproducible environment.

Can you create another PR which moves the DFSelectionSetBenchmark into the performance package and renames into DFSelectionSetPerformance?

We can merge this PR first and it gives us a clear baseline for the improvements. Thanks a lot

Nice to see, created the PR here: #3856

@timward60 timward60 force-pushed the timward/data-fetching-selection-set-immediate-fields branch from 96fab24 to 0868473 Compare March 19, 2025 00:24
@andimarek
Copy link
Member

andimarek commented Mar 19, 2025

@timward60 you can see the performance results from master tests here: https://github.com/graphql-java/graphql-java/tree/master/performance-results

@dondonz dondonz added this to the 23.x breaking changes milestone Mar 20, 2025
bbakerman
bbakerman approved these changes Mar 20, 2025
View reviewed changes
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

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

Thanks very much for this

@bbakerman bbakerman merged commit 9a931ba into graphql-java:master Mar 20, 2025
1 check passed
@dondonz dondonz added the performance work that is primarily targeted as performance improvements label Mar 20, 2025
@timward60 timward60 deleted the timward/data-fetching-selection-set-immediate-fields branch July 9, 2025 05:59
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

@andimarek andimarek andimarek approved these changes

Assignees

No one assigned

Labels

performance work that is primarily targeted as performance improvements

Projects

None yet

Milestone

23.0 breaking changes

Development

Successfully merging this pull request may close these issues.

4 participants

@timward60 @andimarek @bbakerman @dondonz