Skip to content

Convert NoOpValueCache#get return value to constant exception #108

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.

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

Closed
wants to merge 1 commit into from

Conversation

jord1e
Copy link

@jord1e jord1e commented Feb 12, 2022

Describe the bug
A UnsupportedOperationException was being created on every NoOpValueCache#get call.

I tested one of my queries with some dataloaders: 100.000 query operations over three waves. The result was 700.000 UnsupportedOperationException initializations!

Imagining the unnecessary allocations at scale is astounding

JMC view:
afbeelding

Stacktrace:
afbeelding

To Reproduce
Use DataLoader+GraphQL Java with the NoOpValueCache and analyze with JMC or JFR (jdk.JavaExceptionThrow event)

@kaqqao
Copy link

kaqqao commented Feb 12, 2022

Stack traces are captured when an exception is created. Throwing the same instance will result in incorrect traces. Look at JVM classes, e.g. UnmodifiableCollection#add.
Exceptions are supposed to be created in exceptional cases, not in hot paths.

Or am I misunderstanding the situation? 😶

@jord1e
Copy link
Author

jord1e commented Feb 13, 2022

will result in incorrect traces [...] Or am I misunderstanding the situation?

@kaqqao No, you aren't

The thing is that the exception is discarded right after checking if the key is cached:

Thus it does not matter.

@bbakerman
Copy link
Member

Thanks for this PR - this is important. It was not meant to be allocating any memory

I think we can take this further.

I created #109 so that it can short circuit out of the value cache which makes it faster by default

@jord1e
Copy link
Author

jord1e commented Feb 14, 2022

No. #109 looks good, glad my profiling helped bring this issue to light.

I will close this PR as it is no longer necessary.

@jord1e jord1e closed this Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants