-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Caching parse and validate by default #4121
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
base: master
Are you sure you want to change the base?
Conversation
Test Results 326 files 326 suites 3m 31s ⏱️ Results for commit c42baa7. ♻️ This comment has been updated with latest results. |
|
I have manually tested a consumer where Caffiene is and is not on the classpath or I can confirm the Caffeine detection code works as expected and it does NOT load any Caffeine code unless its present and hence it works as a No Op when caffeine is NOT present but caches when it is present |
| return CompletableFuture.completedFuture(cacheEntry); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its surprisingly little code
|
|
||
| CaffeineDocumentCache(boolean isCaffeineAvailable) { | ||
| if (isCaffeineAvailable) { | ||
| CaffeineDocumentCacheOptions options = CaffeineDocumentCacheOptions.getDefaultJvmOptions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice it never loads an Caffeine code unless Caffeine is on the class path
| private static CaffeineDocumentCacheOptions defaultJvmOptions = newOptions() | ||
| .expireAfterAccess(EXPIRED_AFTER_ACCESS) | ||
| .maxSize(MAX_SIZE) | ||
| .build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an area for more discussion. Should we have simple defaults or try to guess based on a memory heuristic??
src/main/java/graphql/GraphQL.java
Outdated
| private ExecutionIdProvider idProvider = DEFAULT_EXECUTION_ID_PROVIDER; | ||
| private Instrumentation instrumentation = null; // deliberate default here | ||
| private PreparsedDocumentProvider preparsedDocumentProvider = NoOpPreparsedDocumentProvider.INSTANCE; | ||
| private PreparsedDocumentProvider preparsedDocumentProvider = new CachingDocumentProvider(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caching is now on by default. If you dont want it use NoOpPreparsedDocumentProvider.INSTANCE which still exists
This is some early work to cache parsed and validated documents by default.
This is discussed in more detailed here #4107