Conversation
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
July 23, 2022 22:15
940dca6 to
2f271e9
Compare
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
August 27, 2022 03:02
ac8e049 to
08b24cd
Compare
ben-manes
force-pushed
the
master
branch
17 times, most recently
from
September 10, 2022 01:17
a337c19 to
48d5246
Compare
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
September 26, 2022 02:50
53f222c to
00b1503
Compare
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
October 2, 2022 09:40
73f40d3 to
6abd616
Compare
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
October 14, 2022 06:02
a7d31f5 to
d74756d
Compare
UnboundedLocalCache.remap and computeIfPresent applied the user function at the top of the map callback with no guard, so a throw propagated before discardRefresh ran and an in-flight refresh survived to overwrite the entry. BoundedLocalCache.remap discards the refresh in its catch before rethrowing; the unbounded sibling was unpinned drift. Wrap the callback so a thrown user function discards the racing refresh and rethrows, matching the bounded cache. compute and merge route through remap, so remap and computeIfPresent cover the family; computeIfAbsent's new-node throw path is tracked separately.
Under store-by-value, LoadingCacheProxy.get/getAll stored the caller's key and JCacheLoaderAdapter.load/loadAll/reload stored the loader's value without copying, unlike every other store site. A mutable key mutated after a read-through get corrupted the stored key, and a loader that retained its returned value could later mutate the stored entry. The RI copies both key and value, and Caffeine's own invoke path copies the loaded value, so read-through was the outlier. Copy the key at the proxy boundary and the value in the adapter before wrapping it in Expirable. Create the copier once in the cache factory and pass it into CacheProxy and the adapter instead of building it twice.
putIfAbsentNoAwait published the lazy-expiry EXPIRED event and counted the eviction before the abortable writer and value copy, unlike put. On an expired prior a failing writer then aborted the compute with the expired mapping still present and the listener having already seen EXPIRED, so the next touch re-fired it (duplicate event and eviction) and left a stranded synchronous-listener future. The writer also ran before copyOf(value), dirtying the write-through store on a failed putIfAbsent with a non-serializable value. Reorder to copy, write, then publish the expired prior, matching put so an aborted attempt leaves the prior mapping and its events untouched; the writer now throws before any synchronous future is pended.
postProcess published a lazily-expired prior's EXPIRED event and counted the eviction before the action switch's abortable writer and copier. On an expired prior whose processor creates, loads, or deletes, a failing writer (or non-serializable value) then aborted the compute with the expired mapping still present and the listener already notified, so the next touch re-fired it: duplicate event and eviction for one expiration. Keep classifying the expired prior as absent for the switch, but defer publishing its EXPIRED and counting the eviction until after each terminal action's writer/copier, matching put. READ and UPDATED cannot co-occur with an expired prior, and invoke's writer-before-copy order is unchanged.
The access-expiry touch (get/getAll/EntryIterator/getOrLoad) runs outside the entry lock, so a concurrent replace lets the stale setExpiresAfter(key, ...) move the new entry's native timer. It is benign: getExpiryForAccess() is value-independent, so the applied duration equals what a real get of the replacement would set (a legal get-after-put serialization), the authoritative Expirable.expireTimeMillis is only written on the held instance so reads stay correct, and the effective expiry is the earlier of native/wrapper (both legal ends). A fix would need a computeIfPresent bin-lock per access read, the wrong trade against best-effort expiry. Catalogued so audits stop re-raising J3/F3/F4.
- serialize() throws CacheException instead of UncheckedIOException, so a store-by-value put of a non-serializable value surfaces the type that deserialize() and Cache.put's contract already use (matches cache2k). - addKeyValueTypes resolves key-type/value-type via the context classloader (was the adapter's own module loader) -- the spec idiom, since Caching.getDefaultClassLoader() is the TCCL and FactoryBuilder resolves the customization classes through it too. - Catalogue both, plus the intentional from() ConfigException bubble-up, in jsr107-conformance.md.
The per-key catch was EntryProcessorException-only, so a non-EPE failure -- e.g. a non-serializable key's copyOf(key) CacheException, outside the EPE-wrapping remap -- escaped and aborted the whole batch. Capture it as that key's result instead, wrapping a non-EPE once in EntryProcessorException (an existing EPE passes through). Matches the RI and other adapters which all isolate per-key. Single-key invoke is unchanged (raw CacheException for a bad key).
JCacheLoaderAdapter.expireTimeMillis called duration.isZero() without a null guard, so a getExpiryForUpdate() of null (the CreatedExpiryPolicy / AccessedExpiryPolicy / default EternalExpiryPolicy behavior) NPE'd into the catch and returned MAX_VALUE, making a refreshed finite-expiry entry eternal (plus a WARNING per refresh). The helper now takes a boolean created, mirroring CacheProxy.getWriteExpireTimeMillis: a null or throwing update returns the MIN_VALUE "unchanged" sentinel that reload remaps to the prior expiry; creation keeps null/throw as eternal so the loaded entry is not lost. reload is an update (it receives oldValue), so this matches the put/replace update paths, which also leave a null or throwing update expiry unchanged; only the insert path is eternal-on-throw.
Under store-by-value, putNoCopyOrAwait copies the key and value before the atomic compute, so a non-copyable entry threw out of the putAll store loop and skipped the single post-loop awaitSynchronous. The entries already committed had added their synchronous CREATED/UPDATED futures to the pending ThreadLocal, which then leaked to the thread's next operation (over-wait only; the entries are committed and the events will be delivered). Wrap the store loop in try/finally with awaitSynchronous in the finally, mirroring getAll. Every committed putAll publish corresponds to a real mutation, and the up-front writeAll-failure path publishes nothing, so the await is always correct. removeAll(Set) shares the loop shape but removeNoCopyOrAwait uses the raw key (no copyOf), so it has no mid-loop throw vector and is unaffected.
CacheProxy.get/getAll propagated a store-by-value copier's non- CacheException RuntimeException raw, while LoadingCacheProxy wrapped it, so the same call surfaced a different exception type depending on whether the cache was read-through. Per JSR-107 each operation @throws CacheException on a problem fetching/doing, and the RI's read-copy (fromInternal) throws CacheException. Fold the copy helpers behind a single copyOf that rethrows NPE/ISE/CCE/CacheException raw and wraps any other RuntimeException in CacheException, so every method (get/getAll/getAnd*/put*/replace*/ iterator) is consistent; the copier is the only user-pluggable non-CacheException vector. copyOf is strict (non-null parameter) so NullAway enforces the assumed non-null at each call site, and the three getAndX prior-value returns guard the null explicitly. The loader adapter's own copy stays CacheLoaderException (a CacheException subtype, correct for the load flow).
IndexedCache.get loaded a value and stored it by the value's own index keys without checking that the requested key was among them. A loader that returns a value not indexed by the requested key never populates that key's mapping, so every subsequent lookup misses, reloads, and re-puts -- a silent permanent cache-bypass with REPLACED churn. Assert the loaded value indexes back to the requested key so bad loader usage fails fast. Independent key groupings remain the caller's responsibility, so the cross-primary uniqueness race and load-then-put staleness are left as documented properties of the minimal sample.
The paper's Algorithm 1 routes eviction on the real-valued S.size >= 0.1 * cache size, so an empty small queue never selects itself -- 0 >= 0.1*C is false for any cache size. Casting maximumSize * percentSmall to a long floors that threshold to zero for a small cache (maximumSize <= 9 at the default 10%) or at percent-small = 0, turning the routing into sizeSmall >= 0, which is always true. With small empty and the cache full, evict() then targets the empty small queue on every iteration and the insertion loop spins forever. Clamp maxSmall to at least one slot so a non-zero configured fraction never vanishes, restoring the paper's non-zero threshold. This changes maxSmall only where it floored to zero; every size with 0.1*C >= 1 is untouched, so validated hit rates are unchanged.
A node's frequency started at zero, counting only re-accesses after
insertion. The Hyperbolic paper (ATC'17, Fig 1 / Eq 1) defines an
item's priority as n_i divided by its time in cache, where n_i is the
request count since it entered -- the inserting access counts, so a
new item enters at high priority ("temporary immunity") and decays
toward its true popularity. Caffeine's zero-based frequency gave a
fresh entry a hyperbolic priority of 0.0, the lowest, so sampled
Hyperbolic evicted new arrivals first -- the exact inversion of the
paper.
Initialize frequency to 1 at insertion so it means "requests since
entering", matching the paper's n_i. This is order-preserving for the
LFU and MFU selections (a uniform +1 shift leaves argmin/argmax
unchanged), so only Hyperbolic's eviction decisions change; FIFO, LRU,
MRU, and RANDOM do not read frequency.
IndicatorClimber and MiniSimClimber set their starting window from settings.percentMain().getFirst(), but HillClimberWindowTinyLfuPolicy builds one policy per percent-main element and hands each its own value. A climber for any element after the first therefore dead- reckoned its window adaptations from the first element's baseline and never corrected, so a percent-main sweep produced wrong curves for every point but the first (the default single-element [0.99] masked it). Thread the per-instance percentMain through HillClimberType.create so each climber baselines from its own element. The eight dead-reckoning climbers ignore it; only the two absolute-target climbers use it. The default is byte-identical (create(0.99, ...) still yields 1 - 0.99); only multi-element sweeps change. Ad hoc loop/corda checks: swept starts now converge to the climber's intended window (loop minisim at an 80%-window start 9.9% -> 45.9%) instead of stranding at the wrong baseline.
SimpleClimber is a readable reference for the production hill climber, so it should represent the algorithm at full quality -- simpler only by shedding library machinery, not by being a weaker climber. Two algorithmic improvements BLC has were missing: - Direction: it started every cache shrinking the window. BLC grows the window first for small caches (setStepSize's positive sign at maximum <= SMALL_CACHE_THRESHOLD, from "Improve hill climber adaptation at small cache sizes"). Start increaseWindow = isSmallCache. - Freeze: the large-cache path set sampleSize = Integer.MAX_VALUE once the step decayed below 0.01, which made adapt()'s restart unreachable -- a permanent stall through any later workload shift. BLC never freezes; it keeps decaying the step and revives it on a hit-rate change. Drop the freeze and floor sampleSize at 1 so the restart stays reachable. The default sample-decay-rate is 1.0, so the period was already fixed like BLC's; only the freeze and initial direction diverged. Verified on the corda_large + 5x loop + corda_large stress trace and a spread of bundled LIRS traces at 512: net positive (corda +2.05, sprite +1.32, multi3 +1.17; small losses within noise), tracking product.Caffeine. Pinned by SimpleClimberTest.
Eviction metric (hit rate unaffected -- hits/misses are recorded independently): - ArcPolicy: case (i) of onMiss deletes an LRU T1 page directly from the cache but never recorded it, so a scan reported ~0 evictions while thousands were paged out. Record it, like the evict() helper. - TwoQueuePolicy: reclaimFor recorded an eviction on the OUT-queue overflow -- dropping an already non-resident identifier -- instead of the IN->OUT page-out that actually evicts a resident entry. Move the record to the page-out, so the count no longer trails by OUT hits and residual OUT occupancy. Reader sibling consistency: - CambridgeTraceReader ignored the MSR disk-number column, so concatenated multi-volume files aliased the same block across disks. Namespace keys by disk like the other SNIA block readers (single-file runs have a constant disk number, so their hit rate is unchanged). - LibCacheSim csv/twitter readers emitted weight 0 for a zero-size object (making it eviction-exempt); floor at 1 to match the binary sibling reader.
The facade's asMap().replace(K, V, V) short-circuited on the old-value guard first, so replace(null, null, v) and replace(k, null, null) returned false where Guava throws. Guava's LocalCache.replace checks the key and new value for null before considering the old value; do the same so the null corners reject rather than silently no-op. The existing null-old-value case (replace(k, null, v) -> false) is unchanged. Pinned by a CaffeinatedGuavaTest parity case run against both the adapter and a real Guava cache.
The f3a5035 fix rescheduled a raced write's drain only when clear() drained every entry under the lock (entries.isEmpty()). When the under-lock loop bails early with stragglers remaining, it relied on each straggler remove(key) reaching afterWrite to reschedule. A concurrent thread that already removed those keys makes every remove(key) a no-op (computeIfPresent skips the absent mapping), so a write that buffered a task and set drainStatus REQUIRED while its scheduleDrainBuffers bounced off the held eviction lock stays stranded with no processor until the next cache op. Drop the entries.isEmpty() guard so the epilogue always nudges rescheduleCleanUpIfIncomplete (a no-op unless REQUIRED).
advance()'s negative-to-non-negative crossing rebiases the timestamps so the unsigned per-level tick delta is correct. The bias must be 2^63 to map the negative range [MIN_VALUE, -1] strictly below the non-negative range in unsigned space. Biasing by Long.MAX_VALUE (2^63-1) is one short: previousTimeNanos == Long.MIN_VALUE maps to unsigned 2^64-1 instead of 0, so every wheel delta is <= 0, the loop breaks at level 0, and the whole advance is a no-op (background expiration stalls until the next advance with a non-degenerate previous; the read path stays exact). Bias by Long.MIN_VALUE, whose bit pattern is 2^63.
Mirror BoundedLocalCache's evict-before-callback ordering in the jcache invoke path. A lazily-expired prior is reconciled inline before the processor runs: it publishes EXPIRED and counts the eviction up front, then the processor sees the entry as absent. On any Throwable from the processor or a write-through CacheWriter, the expired prior's removal is committed, its synchronous EXPIRED listener is awaited, and the failure is rethrown after the compute returns (catch-commit-rethrow) via processorFailure -- an Error as-is, otherwise an EntryProcessorException per Cache.invoke's contract. So a failed invoke still fires exactly one EXPIRED and one eviction; the expiration is a clock fact, not contingent on the operation succeeding. postProcess is now expiry-free: a null prior means the entry was absent, so READ/UPDATED (which require a live prior) never observe one and their requireNonNull is a proven invariant. This drops the hoisted null that made READ/UPDATED look NPE-prone (superseding #1992) and the currentTimeMillis == 0 re-read sentinel. Tests: postProcess reduced to 2-arg replay plus an eternal invoke guard (isEternal shields hasExpired from a negative-clock overflow); and invoke-level guards for an expired prior under create, remove, a thrown RuntimeException and Error, an awaited synchronous listener on the async executor, and EXPIRED-before-CREATED ordering. Verified with :jcache:test and :jcache:tckTest.
Policy.eviction().coldestWeighted/hottestWeighted document Long.MAX_VALUE as the "disregard the limit" sentinel, but WeightLimiter accumulated the scanned weight with Math.addExact, which throws ArithmeticException once the running sum crosses 2^63 (reachable only at theoretical scale: a near-MAXIMUM_CAPACITY weighted cache with >2^32 resident entries). Both operands are non-negative, so a wrapped-negative sum is exactly the overflow signal; clamp to Long.MAX_VALUE there. Once saturated the limit check stays true for a MAX_VALUE limit (all entries returned, as documented) and false for any finite limit (scan stops).
UnboundedLocalCache.remap discarded a refresh unconditionally on the absent+null path, so replaceAll racing a concurrent remove + re-refresh killed a token registered after the removal — where BoundedLocalCache returns early without discarding for a non-creating caller. A vanished key is a skip, not a mutation, so the refresh it raced (none) must not be cancelled. Thread a computeIfAbsent flag (false for replaceAll and computeIfPresent, true for compute and merge) and return early without discarding on the absent branch when creation is disallowed; a creating caller that returns null still discards, as before.
doComputeIfAbsent's new-node path discards a racing refresh on a clean value or null return but deliberately preserves it when weigher/expiry throws: a throw aborts the creation without installing a value, and an independent in-flight refresh may still legitimately populate the absent key. This is correct, not a gap vs remap (which discards on a throw only because it doubles as the refresh-completion self-clean); adding the finally here would make a failed computeIfAbsent abort an unrelated refresh. Pin it so a future "consistency fix" can't invert it, and document the intentional asymmetry. Also carries the by-design note for bulk getAll not discarding an unloaded key's refresh (a non-linearizable side-load has no atomic absence instant to justify a discard).
Pacer.schedule only cancelled the future it replaced on the not-yet-overdue branch; an overdue-but-still-pending future (a delayed task, or a fire-time-rejected scheduler future) was replaced without cancelling, so its task could still fire a redundant (idempotent) maintenance pass. Cancel whenever a non-null future is replaced. Also documents the accepted fire-time scheduler-rejection best-effort: a JDK delayedExecutor limitation orphans the pacer future on a fire-time executor rejection (self-heals within one cycle; within the amortized-expiration envelope).
…lock The JSR-107 CacheWriter contract requires the non-batch writer methods to be "atomic with respect to the corresponding cache operation", but CacheProxy.remove(K)/getAndRemove(K) called writer::delete before an unconditional cache removal -- so a racing same-key put interleaved between the store delete and the cache removal, leaving the store's value while the cache went absent (a conformance violation; adversarial F21, distinct from the putIfAbsent/invoke ordering fixed earlier). removeNoCopyOrAwait now takes a publishToWriter flag and uses compute, firing the writer under the per-key bin lock atomically with the removal (mirroring putNoCopyOrAwait); it still fires unconditionally for an absent key. This matches the RI and every peer (Ehcache3 uses the same writer-inside-compute mechanism); only Coherence's in-process localcache shares the old window. removeAll stays on the batch deleteAll, which the spec explicitly does not require to be atomic. Honoring deleteAll's residual collection on a clean return (the RI's reading) would make removeAll a cache no-op for any writer that does not clear the collection -- every mock and most naive writers -- so remove-all-on-success is kept; the residual is honored on the throw path. A per-key delete loop is permitted but neither required nor more correct (Ehcache3 and Coherence-partitioned also batch). Pinned by a 2-thread interleaving test and a non-clearing-writer guard in CacheWriterTest. Catalogued in jsr107-conformance.md, with the write-through-under-compute convention in the adapter rule and the asyncReload refreshes-bin-lock context added to synchronization.md. Verified with :jcache:test and :jcache:tckTest.
CacheProxy recorded the operation duration on every write and delete path even when nothing was stored or removed, while the paired put/removal counter only moves on an actual mutation, so getAveragePutTime and getAverageRemoveTime were skewed high by no-op calls. Gate each timing call on the same condition as its counter, matching the two-arg replace(K,V), the iterator remove(), and the JSR-107 reference implementation.
putIfAbsent derived its hit/miss from whether a value was stored, so an absent key under a zero-creation-expiry -- where the new value is immediately expired and not stored -- recorded a hit instead of a miss. The JSR-107 statistics table counts a putIfAbsent miss whenever the key is absent, independent of whether anything was stored; getAndPut already classifies off prior-presence and gets this right. Thread prior-presence out of putIfAbsentNoAwait (a present flag set in the live-mapping branch) and classify the hit/miss by it, leaving the put and put-time gated on the store. Only the zero-creation-expiry configuration is affected; the normal, present, and expired-under-normal cases were already correct. Pinned by a strengthened JCacheCreationExpiryTest case asserting the absent zero-creation-expiry putIfAbsent records a miss, not a hit.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
No description provided.