Skip to content

Avoid no-op field fetch instrumentation allocations#4391

Merged
andimarek merged 2 commits into
masterfrom
review-4375-chained-noop-allocation
May 20, 2026
Merged

Avoid no-op field fetch instrumentation allocations#4391
andimarek merged 2 commits into
masterfrom
review-4375-chained-noop-allocation

Conversation

@andimarek
Copy link
Copy Markdown
Member

@andimarek andimarek commented May 20, 2026

Summary

  • include the original Avoid no-op field fetch adapter allocation #4375 fix to return the canonical field-fetching no-op when legacy field-fetch instrumentation returns SimpleInstrumentationContext.noOp()
  • avoid chained field-fetch context/list allocation when all chained instrumentations are canonical no-ops
  • return the sole real field-fetch context directly when no-op entries surround it
  • preserve chained callback behavior when multiple real field-fetch contexts are present

Verification

  • ./gradlew test --tests graphql.execution.instrumentation.InstrumentationDefaultMethodsTest --no-build-cache
  • ./gradlew test --tests 'graphql.execution.instrumentation.*'

@andimarek andimarek requested a review from bbakerman May 20, 2026 05:45
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026 •
edited
Loading

Test Report

Test Results

Java Version Total Passed Failed Errors Skipped
Java 11 5845 (+8 🟢) 5789 (+8 🟢) 0 (±0) 0 (±0) 56 (±0)
Java 17 5845 (+8 🟢) 5788 (+8 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 21 5845 (+8 🟢) 5788 (+8 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 25 5845 (+8 🟢) 5788 (+8 🟢) 0 (±0) 0 (±0) 57 (±0)
jcstress 32 (±0) 32 (±0) 0 (±0) 0 (±0) 0 (±0)
Total 23412 (+32 🟢) 23185 (+32 🟢) 0 (±0) 0 (±0) 227 (±0)

Code Coverage (Java 25)

Metric Covered Missed Coverage vs Master
Lines 29605 3130 90.4% ±0.0%
Branches 8696 1532 85.0% ±0.0%
Methods 7906 1210 86.7% ±0.0%

Changed Class Coverage (2 classes)

Class Line Branch Method
g.e.i.ChainedInstrumentation +0.8% 🟢 ±0.0% ±0.0%
g.e.i.Instrumentation +0.6% 🟢 +25.0% 🟢 ±0.0%

Full HTML report: build artifact jacoco-html-report

Updated: 2026-05-20 07:10:53 UTC

bbakerman
bbakerman approved these changes May 20, 2026
View reviewed changes
Copy link
Copy Markdown
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.

Performance matters but gets harder ands harder to find!

nwjsmith and others added 2 commits May 20, 2026 16:53
@nwjsmith @andimarek
Avoid no-op field fetch adapter allocation
0ae9fad 
The default field-fetching bridge still has to support legacy
beginFieldFetch implementations, but the inherited no-op path should not
allocate a per-field adapter. Return the canonical field-fetching no-op
when the legacy hook returns the canonical no-op context, while still
adapting real legacy contexts.
@andimarek
Avoid chained no-op field fetch contexts
76ec535
@andimarek andimarek force-pushed the review-4375-chained-noop-allocation branch from 39b95dd to 76ec535 Compare May 20, 2026 07:01
@andimarek andimarek enabled auto-merge (squash) May 20, 2026 07:05
@andimarek andimarek merged commit 811e7f4 into master May 20, 2026
11 checks passed
@andimarek andimarek mentioned this pull request May 20, 2026
Closed
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andimarek @bbakerman @nwjsmith