Skip to content

Fix inconsistent weigher() javadoc in CacheBuilder#8298

Open
daguimu wants to merge 1 commit intogoogle:masterfrom
daguimu:fix/cachebuilder-weigher-javadoc-inconsistency-1690
Open

Fix inconsistent weigher() javadoc in CacheBuilder#8298
daguimu wants to merge 1 commit intogoogle:masterfrom
daguimu:fix/cachebuilder-weigher-javadoc-inconsistency-1690

Conversation

@daguimu
Copy link
Copy Markdown

@daguimu daguimu commented Mar 26, 2026

Problem

The weigher() javadoc in CacheBuilder states that entry weight is used "when determining which entries to evict", contradicting the maximumWeight() javadoc which correctly states that "weight is only used to determine whether the cache is over capacity; it has no effect on selecting which entry should be evicted next."

Root Cause

The weigher() javadoc was written with imprecise wording. The implementation in LocalCache.Segment.getNextEvictable() confirms that eviction candidate selection is based on access recency (LRU via the accessQueue), not entry weight. Weight only affects whether the total capacity threshold has been exceeded, triggering eviction.

Fix

  • Updated the weigher() javadoc to say weight is used "when determining whether the cache is over capacity" instead of "when determining which entries to evict"
  • Applied the same fix to both JRE and Android variants

Impact

Javadoc-only change. No behavioral change. Resolves the documented inconsistency between weigher() and maximumWeight() javadoc.

Fixes #1690

The weigher() javadoc stated that entry weight is used "when determining
which entries to evict", but maximumWeight() javadoc correctly states
that "weight is only used to determine whether the cache is over
capacity; it has no effect on selecting which entry should be evicted
next." The implementation confirms this: getNextEvictable() selects
entries based on access recency (LRU), not weight.

Updated the weigher() javadoc to say weight is used "when determining
whether the cache is over capacity", consistent with maximumWeight().

Fixes google#1690
@chaoren
Copy link
Copy Markdown
Member

chaoren commented Mar 26, 2026

@ben-manes, looks like the Caffeine docs have the same inconsistency:

https://github.com/ben-manes/caffeine/blob/fbadda61eac012f7553e6a62e05ace7acd63f0f1/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java#L473-L474

   * Specifies the weigher to use in determining the weight of entries. Entry weight is taken into
   * consideration by {@link #maximumWeight(long)} when determining which entries to evict, and use

https://github.com/ben-manes/caffeine/blob/fbadda61eac012f7553e6a62e05ace7acd63f0f1/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java#L451-L452

   * Note that weight is only used to determine whether the cache is over capacity; it has no effect
   * on selecting which entry should be evicted next.

@chaoren chaoren added type=api-docs Change/add API documentation status=triaged package=cache P3 no SLO labels Mar 26, 2026
@ben-manes
Copy link
Copy Markdown
Contributor

That is a fair confusion, though "consideration" means its left to maximumWeight to decide how it may be used. That saying it is only used for the capacity limit is the algorithmic details that is the policy's choice of useful information, whereas a Weigher just provides the information. I don't think .weigher(...) should include that detail, but I can see how the word consideration might be too subtle.

We did collaborate to investigate using the weight as part of the eviction decision and shared our findings (paper). There are some modest gains but also adds complexity wrt concurrency, and for an on-heap Java cache it was unclear how beneficial the enhancement would be in practice so we backlogged it.

@chaoren
Copy link
Copy Markdown
Member

chaoren commented Mar 26, 2026

I think these two statements in particular are in conflict:

weight is taken into consideration ... when determining which entries to evict

[weight] has no effect on selecting which entry should be evicted next

Maybe weigher doesn't need to say anything about how the weight is going to be used, just that it would used by maximumWeight. Declaring that maximumWeight would use it to determine which entries to evict directly contradicts the documentation on maximumWeight. I think we should at least remove that part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistency in CacheBuilder.weigher() and maximumWeight() javadoc

3 participants