LDEV-6327 rewrite near cache as map+queue (also fixes LDEV-4413 write-side aliasing)#19
Merged
Merged
Conversation
…all" adds hook to inject thread delays for testing purposes
With goal being not sharing live object refs to the underlying "to-be-cached" objects across NearCacheEntry objects fixes test introduced in prior commit resolves lucee#13
a0f552a to
063aae6
Compare
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
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.
The bugs
Two related correctness bugs in the redis cache near-cache layer:
cacheGetreturns a shared reference to the cached value. Callers (or other concurrent readers) can mutate it, corrupting the cached state. David Rogers's PR copy NearCacheEntry values as-if they had been materialized from cache #16 fixes the read side; the write side (concurrent puts of mutable objects) was unfixed.Storage.getreturns the older entry. Reads can see stale writes for up to the drain window — deterministic 78/100 repro on master.This PR also resolves
thread.join(), sessionCluster=trueapplication action="update"losing session varssessionInvalidate()not clearing RedisThe root cause
Storagewas the wrong data structure:Every single
cacheGetallocated anObject[]snapshot of the deque and linear-scanned it. With the deque holding 1000 pending entries, that's ~50µs of pure CPU per get — slower than just hitting Redis. The near cache was actively slowing things down vs not having one.JFR on master, full bench suite:
Arrays.copyOf(Object[])— 59% of CPU (insidedeque.toArray)Storage.equals(byte[], byte[])— 25% of CPUConcurrentLinkedDeque.toArrayInternal— 9% of CPUObject[]— 95% of all allocation pressureThe fix
Replace the deque with a map keyed by content-equality wrapper, plus a separate queue for drain order:
Storage.put:val→byte[]at call time. Captures an immutable snapshot before the caller can mutate. Closes LDEV-4413 write-side.entries.put(wkey, entry)overwrites the prior entry. Closes LDEV-6327 — there's no stale entry left to find.drainQueue.offer(wkey)schedules the write to Redis.Storage.get:entries.get(wrapper). O(1). NoObject[]. No scan.Drain thread:
drainQueue, re-reads the latest entry from the map (a newer put may have overwritten), writes pre-serialised bytes directly to Redis via a newputBytesoverload (no re-serialise), conditional-removes from the map.Three commits, telling the story in order:
add failing test(PR copy NearCacheEntry values as-if they had been materialized from cache #16)copy NearCacheEntry values as-if they had been materialized from cache(PR copy NearCacheEntry values as-if they had been materialized from cache #16, fixes LDEV-4413 read side)LDEV-6327 replace near cache deque scan with map+queue(fixes LDEV-6327 + LDEV-4413 write side)David's authorship preserved on his commits.
The evidence
Correctness (deterministic testbed scenarios, 16-thread contention):
Throughput (ABBA, 100k cycles, 16 threads, full suite):
No regressions anywhere. The multipliers measure the bug-path tax in isolation; the synthetic 100k+ ops/s assumes the JVM is dedicated to cache calls. In production, expect lower per-op CPU, ~10× less GC pressure, and better tail latency under cache contention — not literal 60× application-level speedup.
JFR on the same suite confirms master spends ~93% of CPU on the deque-scan path; this PR has 0% there. Full bench suite finishes in 55s on this PR vs 185s on master.
Tests: extension test suite (
./test.bat) — 27/27 pass on Lucee 7.1, 22/22 on Lucee 7.0. PR #16's own regression test (NearCacheEntriesAreCopiedOnReadPriorToWriteCommit) still passes.Trade-offs
cachePutnow serialises on the caller thread instead of the drain thread. Necessary for the write-side fix — the snapshot must be taken before the caller can mutate. Bench shows pure-put throughput goes up anyway, because the drain stops being a bottleneck.Storage.putnow throws IOException on serialise failure. Master swallowed these in the drain thread. Fail-fast is correct; signature unchanged at the publicCache#putboundary which already declaresthrows IOException.ByteArrayWrapperallocated percacheGetfor the map lookup. ~16 bytes, JIT likely stack-allocates via escape analysis. Trivial vs theObject[]snapshot of 1000 entries this replaces.