State is passed explicitly to instrumentation and parameters are NOT mutated #2769
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…passing in state to the instrumentation fields
…licitly-to-instrumentation
…licitly-to-instrumentation # Conflicts: # src/main/java/graphql/execution/instrumentation/ChainedInstrumentation.java
bbakerman
changed the title
…licitly-to-instrumentation
|
Hi. Wondering if there is an update on this as we're very keen to see the impact on object allocation. Thanks :) |
|
I am waiting on a final review from @andimarek - Andi? |
|
@andimarek did you get a chance to review this PR? |
|
Hello @andimarek and/or @bbakerman! Did you manage to follow up on this PR? |
…licitly-to-instrumentation # Conflicts: # src/main/java/graphql/execution/Execution.java
|
I just fixed up the merge conflict on this PR - this will be merged into 19.0 - 19.0 is to be released very soon - probably next week |
andimarek
approved these changes
Jul 26, 2022
Comment on lines
+75
to
+79
| public InstrumentationContext<ExecutionResult> beginExecution(InstrumentationExecutionParameters parameters) { | ||
| // these assert methods have been left in so that we truly never call these methods, either in production nor in tests | ||
| // later when the deprecated methods are removed, this will disappear. | ||
| return Assert.assertShouldNeverHappen("The deprecated " + "beginExecution" + " was called"); | ||
| } |
There was a problem hiding this comment.
FYI this is a breaking change for eg, a downstream ExecutionStrategy implementation that calls beginExecution(parameters) and needs to be changed to call beginExecution(parameters, parametersInstrumentationState). May want to update the 19.0 release notes which claim no breaking changes.
There was a problem hiding this comment.
thanks a lot @josephlbarnett ... will update the release notes
josephlbarnett
added a commit
to josephlbarnett/graphql-kotlin
that referenced
this pull request
Jul 27, 2022
FlowSubscriptionExecutionStrategy needs to be updated to pass instrumentationState to instrumentations per graphql-java/graphql-java#2769
josephlbarnett
added a commit
to josephlbarnett/graphql-kotlin
that referenced
this pull request
Jul 28, 2022
FlowSubscriptionExecutionStrategy needs to be updated to pass instrumentationState to instrumentations per graphql-java/graphql-java#2769 ValidationError string changed slightly so adjust assertions.
josephlbarnett
added a commit
to josephlbarnett/graphql-kotlin
that referenced
this pull request
Jul 28, 2022
FlowSubscriptionExecutionStrategy needs to be updated to pass instrumentationState to instrumentations per graphql-java/graphql-java#2769 ValidationError string changed slightly so adjust assertions.
samuelAndalon
pushed a commit
to ExpediaGroup/graphql-kotlin
that referenced
this pull request
Jul 28, 2022
FlowSubscriptionExecutionStrategy needs to be updated to pass instrumentationState to instrumentations per graphql-java/graphql-java#2769 ValidationError string changed slightly so adjust assertions.
dariuszkuc
pushed a commit
to dariuszkuc/graphql-kotlin
that referenced
this pull request
Aug 5, 2022
FlowSubscriptionExecutionStrategy needs to be updated to pass instrumentationState to instrumentations per graphql-java/graphql-java#2769 ValidationError string changed slightly so adjust assertions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As per #2743 - object allocation at scale can be problematic because in a chained instrumentation call we create new
instrumentation parametersper chained call and we create a chainedInstrumentationContextobject (list of all the otherInstrumentationContextobjects) which gets held in memory for a long time and is not eligible for GC.This PR starts to change
Instrumentationso thatInstrumentationStateis passed into each method direct (no need for new objects to be allocated)InstrumentationContextcan be null and hence no object allocation is needed here eitherAll the places that received a
InstrumentationContexthave been changed to swap in a static NooP object (singleton) and hence they dont change a lot in a consuming sense.This PR as is shows that ALL the tests pass - that is the new methods are delegated back to the old no problems.
Some important decisions I made
I used
nulland not Optional for the emptyInstrumentationContextcase - why? Because there is no object allocation involved in thesomecase versus the empty case.The
instrumentXXXmethods have been changed to take inInstrumentationStatefor consistency reasons with all the others even if the memory pressure problem doesnt apply to them as muchThe
instrumentXXXmethods MUST return a non null object as before - I put@Nonnullannotations to this betterThe ChainedInstrumentation class has been updated to copy with null contexts being returned returned AND it only uses the new methods with state and never the old.
Still TODO
I have not updated ALL the provided instrumentations to take advantage of the new methods. I think this is best dont in a another PR to reduce the scope of this already large PR. They all work unchanged so lets not make it more complicated than it needs to be and we can improve it via another PR