[grid] Migrate cache builder to use Caffeine #15547
Draft
+141
−76
Conversation
This file contains hidden or 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
Signed-off-by: Viet Nguyen Duc <[email protected]>
CI Feedback 🧐(Feedback updated until commit cb4a6a8)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
ben-manes
reviewed
Apr 2, 2025
Comment on lines
93
to
+94
| Cache<String, CompletableFuture<PreparsedDocumentEntry>> cache = | ||
| CacheBuilder.newBuilder().maximumSize(1024).build(); | ||
| Caffeine.newBuilder().maximumSize(cacheSize).build(); |
There was a problem hiding this comment.
should this be an AsyncCache or is the intent to retain failed futures?
|
Can you share some context for this? Why do we need this? |
|
This relates to #15370 where we try to see how caffeine could resolve something better than guava. |
|
Hard to tell from the thread as an outside observer, but is that thread’s issue that removal notifications occur sometime after expiration? In guava/caffeine the default is to evict as part of maintenance triggered by other activity, so the explicit cleanUp calls are required for promptness. Caffeine offers a scheduler option for a thread that does this based on the next ttl event. Since the removalListener is called asynchronously, caffeine also offers an evictionListener if you need it as part of the eviction’s atomic map operation. Neither provides linearizable invalidateAll so in-flight loads would be skipped. Guava’s loads are never linearizable whereas Caffeine’s are except for that method because otherwise we’d need to fork ConcurrentHashMap which suppresses initial loads during a traverse. but I don’t know if that’s helpful or if I grok the issues |
|
@diemol the javadoc of Guava's More details here: https://github.com/google/guava/blob/master/guava/src/com/google/common/cache/CacheBuilder.java#L49 |
joerg1985
requested changes
Apr 24, 2025
| @@ -862,7 +855,7 @@ public void stop(SessionId id) throws NoSuchSessionException { | |||
| } | |||
|
|
|||
| private void stopAllSessions() { | |||
| if (currentSessions.size() > 0) { | |||
| if (currentSessions.estimatedSize() > 0) { | |||
There was a problem hiding this comment.
i would suggest to call invalidateAll in any case, as we can not rely on the estimatedSize.
rust/Cargo.Bazel.lock
Outdated
Comment on lines
2
to
3
| "checksum": "f9b7e2302bd7b43ff0899cac357fbbfe0226873f02b610c0eb4246784e14e3b3", | ||
| "checksum": "a896d7a078f1c885fa00c3b9aa5b8727c4f5bdfc58cdc26836fb74ebbdd3c87c", | ||
| "crates": { |
There was a problem hiding this comment.
All changes in this file look not related to the PR.
Signed-off-by: Viet Nguyen Duc <[email protected]> # Conflicts: # MODULE.bazel # java/maven_install.json # rust/Cargo.Bazel.lock
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.
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Motivation and Context
Types of changes
Checklist