perf: fix the L2 cache region-lock convoy on concurrent metadata-heavy load

[netroms]

What this PR changes

Two changes to how the Hibernate second-level cache treats static reference metadata, plus one
config tightening:

1. READ_WRITE → NONSTRICT_READ_WRITE for the reference-metadata regions (options,
   category structures, data elements, org unit hierarchy, indicators, legends, and friends).
2. The hottest regions are declared explicitly in ehcache.xml, sized deliberately and
   stored by reference instead of by serialized copy.
3. Cache TTL is capped at one hour (down from six), so nothing this PR makes possible can
   outlive an hour.

The result on concurrent tracker imports: 26 → 127 rps at 100 concurrent users (4.9x), with
p99 latency 13.9 s → 6.9 s. At 400 users: 26 → 110 rps, p99 39.8 s → 9.9 s. Both builds are
identical except for these commits. Full table and method at the bottom.

The problem: what a cache read actually costs today

Picture a busy instance: a few hundred data clerks submitting tracker events. Every single
submission validates its data values against the same reference metadata: the data elements,
their option sets, the options themselves. With an option-heavy program (think a 14,000-option
diagnosis code list), one submission triggers thousands of cache reads. That is the workload the
second-level cache exists for, and today each of those reads pays two taxes.

Tax 1: one lock per region, paid on every read

READ_WRITE's access strategy guards each region with a single lock. Not per entry, per
region:

// org.hibernate.cache.spi.support.AbstractReadWriteAccess (hibernate-core 5.6.15)
private final ReentrantReadWriteLock reentrantReadWriteLock = new ReentrantReadWriteLock();

Every get takes the read lock; every putFromLoad takes the write lock, the exclusive
one. All 14,000 options share one turnstile, and every query that loads options from the
database grabs that turnstile exclusively to re-insert what it just read.

Here is the part that makes it absurd for DHIS2 specifically: our entities are unversioned,
and READ_WRITE refuses to overwrite an existing cache entry from a load unless the incoming
version is newer:

// AbstractReadWriteAccess.Item#isWriteable (hibernate-core 5.6.15)
public boolean isWriteable(long txTimestamp, Object newVersion, Comparator versionComparator) {
    return version != null && versionComparator.compare( version, newVersion ) < 0;
}   // version == null for every DHIS2 entity → never writeable

So the sequence on every re-load of an already-cached option is: take the region's exclusive
write lock → inspect the entry → decide it must not be written → release the lock → put
nothing. A few hundred concurrent users all paying exclusive-lock prices for a no-op, on the
hottest region in the system. Under load this shows up directly in wall-clock profiles.
On the metadata read/write workload, 14.2% of all JVM wall samples sit parked inside
AbstractReadWriteAccess on the unmodified build; this PR takes it to 0.1%.

What is that machinery for? It exists so that concurrent writes to cached entities cannot leave
readers with torn or resurrected state. That is the right trade for entities that change under
concurrency. Reference metadata does not: an option list changes when an implementer configures
the system, not when a user submits an event. We are paying a per-read cost for a per-write
guarantee on data that is, in production, read millions of times between writes.

Tax 2: every read photocopies the entry

The JCache (JSR-107) spec defaults to store-by-value:

// javax.cache.configuration.MutableConfiguration (cache-api 1.1.0), constructor
this.isStoreByValue = true;   // the spec default

Hibernate creates any region that is not declared in ehcache.xml at runtime, through exactly
that API (JCacheRegionFactory.createCache), so every runtime-created region gets Ehcache's
SerializingCopier installed, and it copies in both directions:

// org.ehcache.impl.copy.ReadWriteCopier (ehcache 3.12.0)
public T copyForRead(T obj)  { return copy(obj); }
public T copyForWrite(T obj) { return copy(obj); }

// org.ehcache.impl.copy.SerializingCopier
public T copy(T obj) {
    return serializer.read(serializer.serialize(obj));  // full serialize + deserialize round trip
}

Reading a cached option does not hand you the cached object. It serializes the stored entry to a
ByteBuffer and deserializes a fresh copy, like a library that photocopies the page every time
someone wants to read it. On every get. On every put. And under READ_WRITE, inside the
region lock's critical section, so tax 2 lengthens exactly the time everyone else spends queued
on tax 1. On the tracker import workload the copier accounts for 227,514 wall samples on the
unmodified build vs 1,086 with this PR.

Change 1: NONSTRICT_READ_WRITE for reference metadata

What READ_WRITE actually guarantees (i.e. what we are giving up)

Worth being precise about, because the guarantee does not come from the read path. An
already-cached entry is never refreshed by a load: Item.isWriteable returns
version != null && …, and our entities are unversioned, so a load can never overwrite it.
Freshness comes entirely from the write path:

// org.hibernate.cache.spi.support.EntityReadWriteAccess#afterUpdate (hibernate-core 5.6.15)
writeLock().lock();
  … if ( !lock.wasLockedConcurrently() )
        putIntoCache( key, new Item( value, version, nextTimestamp() ), session );  // the NEW value

So a metadata write under READ_WRITE runs like this:

step	cache state	what a concurrent reader gets
lockItem(), pre-commit	entry replaced by a soft lock	soft lock is not readable → cache miss → reads the database
database commit	still soft-locked	still misses → database → fresh
afterUpdate(), post-commit	new value written into the cache	fresh, from cache

And the racing load (the one that read the old row just before the write) meets the soft lock on
its way back in and is refused. That refusal is the protection, not a gap: the entry it declines
to overwrite is the correct one, because the write path just put it there. This is what Change 1
gives up, and the trade-off below is the honest price.

One thing READ_WRITE does not protect against, which matters for the TTL discussion: a row
changed outside Hibernate (raw JDBC, a native executeUpdate that does not invalidate, a
direct database edit) leaves a cached entry that no load will ever refresh, for the same
"never writeable" reason. Under READ_WRITE as it ships today, that entry is stale until TTL,
eviction, or the next Hibernate-managed write to that key.

What NONSTRICT_READ_WRITE does instead

NONSTRICT_READ_WRITE removes the lock machinery entirely. Reads are plain cache gets; a write
evicts the key twice, once during the transaction and once after commit, instead of soft-locking
it:

// org.hibernate.cache.spi.support.EntityNonStrictReadWriteAccess (hibernate-core 5.6.15)
public boolean update(...)      { getStorageAccess().removeFromCache( key, session ); ... }  // pre-commit
public boolean afterUpdate(...) { unlockItem( session, key, lock ); ... }                    // post-commit
public void unlockItem(...)     { getStorageAccess().removeFromCache( key, session ); }      //   → evict again

The trade-off, stated precisely

Giving up the locks buys the throughput and costs two well-defined staleness windows. Both
require a write to the metadata. No write, no staleness, ever.

Window 1: the post-write gap. Between the database commit and the second eviction, a
concurrent reader can still be served the previous value. Duration: milliseconds, at worst
seconds. Realistically: an admin renames an option while data entry is running, and a handful of
requests in flight at that exact moment still render the old name. The next request sees the new
one.

Window 2: the stranded key. Rarer, and the reason the TTL matters. A reader that loaded the
previous value from the database can complete its cache-put after the writer's final
eviction, re-inserting the stale value. Nothing refuses that late put; the refusal logic was
precisely the lock we removed. That single key then serves the old value until the next write to
that key, or until TTL expiry, with this PR at most one hour.

Step by step. R is a request thread loading metadata; W is a transaction saving a change to
the same key:

1. W starts its update. It evicts the key (first eviction) and updates the database row. It
   has not committed yet.
2. R gets a cache miss (the key was just evicted) and reads the row from the database. W
   has not committed, so R sees the old value.
3. W commits, then evicts the key again (the post-commit eviction). The cache is now empty for
   this key. Everything is still correct.
4. R completes its cache-put with the value it read in step 2. The cache now holds the old
   value again. Nothing rejects the late put: the check that would reject it is part of the lock
   machinery this PR removes.

The same race as a sequence diagram:

sequenceDiagram
    participant R as R (request thread)
    participant C as L2 cache
    participant DB as Database
    participant W as W (metadata write)

    W->>C: 1. evict(key)
    W->>DB: 1. UPDATE row (uncommitted)
    R->>C: 2. get(key)
    C-->>R: miss (key was just evicted)
    R->>DB: 2. SELECT row
    DB-->>R: old value (W not committed yet)
    W->>DB: 3. COMMIT
    W->>C: 3. evict(key), post-commit
    Note over C: empty, still correct
    R->>C: 4. put(key, old value), nothing rejects it
    Note over C: STALE: every read is now a hit,<br/>no reader returns to the database.<br/>Cleared by the next write to this key, or TTL (max 1 h)

Loading

From step 4 on, every read of this key is a cache hit. No reader goes back to the database, so
nothing corrects the entry. It survives until the next write to this key, or until TTL expiry,
with this PR at most one hour.

How likely is step 4? R must read the row in the final moments before W commits, and its
cache-put must land just after W's post-commit eviction, which follows the commit within about
a millisecond. The race window is a few milliseconds wide, and it exists only while a metadata
write is in flight. Metadata writes are rare events: an option set is edited a few times a
month, not continuously. Between writes the risk is exactly zero. But writes happen during
working hours, when data entry traffic peaks and hundreds of reads are in flight. Over months on
a busy instance, expect the occasional hit. That is why the defence is the bound, not the
odds. The failure mode is one stale option label for at most an hour, once in a while, not data
corruption. Data values never live in these regions; a stale option list cannot change what was
stored, only what one node briefly believes the list looks like.

Concrete worst cases, honestly stated:

• An implementer adds a new option and a user enters data with it seconds later. Window 1 says
  every node sees it within milliseconds-to-seconds. Only a window-2 stranding could pin the old
  option list on one key; in that case imports using the new option could be rejected on that
  node for up to an hour. Re-saving the option set evicts the stale entry immediately.
• Sharing settings ride on these entities. A stranded key can therefore also serve stale
  sharing for up to the same hour; revoking a user's access to a data element is not guaranteed
  to be instantaneous on a key that lost the race. Same bound, same probability structure, and
  the reason the TTL cap is part of this PR rather than a follow-up.

Why one hour

The previous six-hour TTL was inherited from the old configuration. Six hours was invisible
before because nothing lived that long anyway; with the cache actually working, TTL becomes the
real bound on window 2, so this PR lowers it to 3600s across the board. Cost: each cached entry
reloads once per hour, which at measured request rates is noise. If even one hour is too long for
a deployment's ACL expectations, the file is overridable per instance (see Tuning below).

Note that this cap is not only a concession for Change 1. As shown above, a cached entry that
goes stale through an out-of-band write is bound by TTL under READ_WRITE too, so on master
today that bound is six hours. Lowering it to one hour tightens an existing exposure for every
region, whichever strategy it uses.

Period/RelativePeriods stay READ_WRITE deliberately: they are written at runtime by period
generation, which is exactly the workload READ_WRITE is for.

Change 2: declare the hot regions, store them by reference

Store-by-reference cannot be switched on globally: the copier is installed by the JCache layer
for every cache created at runtime (see Tax 2), and the only way out is for the cache to already
exist, i.e. to be declared in ehcache.xml. So this PR declares the measured-hottest
regions explicitly (options, category combos and their option combos, data elements, org units,
users, and the other regions that dominate get-counts under tracker load; the option region alone
out-reads the next region by two orders of magnitude on this workload).

Declared regions hold a reference to the stored entry instead of round-tripping it through a
serializer. This is safe for how Hibernate uses the cache: it never caches live entity objects.
It caches the entity's disassembled state (an array of property values) and reassembles a
fresh entity instance from that array on every cache hit. The cached array is never handed to
application code and never mutated in place, so sharing it by reference cannot leak writes
between sessions.

Declaring the regions also forces the sizing question into the open instead of leaving every
region at the template's one-million-entry default:

region	heap entries	reasoning
CategoryOptionCombo (+ its option links)	500,000	largest table of the bucket on big instances
Option, OrganisationUnit (+ children)	200,000	full national option lists / OU trees with headroom
User, Period, DataElement collections	100,000	comfortably above large-instance counts
Category structures, option sets, indicators	20,000 to 100,000	small tables, modest headroom

These are deliberate over-provisions above the row counts of large real-world databases. The
point is that they are now explicit, bounded, and per-region tunable instead of implicit and
uniform. They are defaults, not claims of optimality; the next section is how to correct them
with data instead of guesswork.

Measured effect

Same-parent comparison: two builds from the same master commit, differing only by this PR's
commits. Gatling closed-model ramp, 10 → 400 concurrent users with 60s plateaus; synchronous
POST /api/tracker single-event imports of an option-heavy event program (14,000-option option
set) interleaved with capture-style metadata reads; batched-payload and metadata write-mixed
profiles as separate cells; Sierra Leone demo DB; n=3 runs per cell; async-profiler wall-clock
profiling on every run.

workload (c100 / c400)	master	this PR
tracker import, single events: rps	26 / 26	127 / 110
tracker import, single events: p99	13,906 / 39,805 ms	6,896 / 9,921 ms
tracker import, batched: rps	13 / 14	36 / 35
tracker import, batched: p99	32,716 / 60,001 ms*	12,529 / 35,401 ms
metadata write-mixed: rps	386 / 368	736 / 686
metadata write-mixed: p99	2,847 / 4,177 ms	534 / 1,506 ms
wall samples parked in region lock (write-mixed)	14.2%	0.1%
wall samples in SerializingCopier (tracker)	227,514	1,086
SELECT statements per request (tracker, c100)	8.6	8.0

* 60,001 ms is the 60 s request timeout: at 400 users the unmodified build's batched imports
time out rather than complete. Run-to-run spread is within ±1 rps on all cells except
write-mixed with this PR (±10).

Anatomy of the timeout. Why does the unmodified build time out rather than just slow down?
Connection hold time. An import holds its pooled connection for its whole transaction,
including all the time its thread spends inside the JVM rather than in the database. At the
400-user plateau the pool runs full in both builds; the difference is what a connection is held
for. On the unmodified build, Hikari reports 78 of 80 connections checked out while
pg_stat_activity shows only about one of them executing SQL. The other 78 sit idle in
transaction, the oldest for around 20 seconds, their owning threads copying cache entries and
queuing on the region lock. Each request occupies a pool slot for tens of seconds, arrivals
outpace turnover, the queue for a free connection compounds, and requests cross the 60 s
timeout. With this PR the holds are about four times shorter (oldest idle-in-transaction around
5 seconds), the same 80-slot pool turns over 2.5x faster (35 rps vs 14), and every request
completes. A residual of 57 idle-in-transaction connections remains and is expected: holding
the connection across the import transaction, including its legitimate in-JVM phases, is how
the import is structured. This PR shortens the hold; it does not claim to remove it.

The two profile rows attribute the gain to its causes: the parked-lock share (visible on the
write-mixed profile) is what Change 1 removes; the copier samples (dominant on the tracker
profile) are what Change 2 removes. Note the last row: SQL volume per request is unchanged. The
PR serves roughly five times the requests at the same eight SELECTs per request; the gain is
in-JVM contention removed, not database work avoided.

Tuning and operations

Nothing here is hardcoded policy. The numbers are defaults with a feedback loop:

• Observe: every region already exposes gets, hits, puts, evictions via
  /api/metrics (Prometheus) when monitoring.ehcache.enabled is on. No restart needed to look.
• Resize: a region showing evictions > 0 while hot is undersized; raise its heap entries.
• Promote: a non-declared region showing sustained high gets is a candidate for an explicit
  declaration (and with it, by-reference storage).
• Retune TTL: staleness expectations are deployment policy; 3600s is the shipped bound, not a
  law.
• Override everything: cache.ehcache.config.file points an instance at its own copy of the
  file: config change and restart, no rebuild.

Test changes

Two integration tests pinned the previous configuration and are updated to pin the new one:
HibernateEhcacheConfigFileTest now asserts both heap-bound paths (a template-inherited region
and an explicitly declared one) instead of assuming every entity region inherits the template;
DataSetMetadataExportServiceQueryCountTest asserts the query count does not grow instead of
exact equality, which is what its own comment says it means (this PR makes the second export
cheaper).

AI Assisted

[netroms]

Rebased onto current master and removed the duplicate scheduler commit entirely — sorry for the noise, @teleivo.

This branch previously carried a standalone subset of your wipe fix so it could be measured on its own. Now that #24803 has merged, that commit is gone from the history rather than merged away: the branch is 3 commits, takes the wipe fix from master, and git diff master..HEAD contains no scheduler code at all.

The rewrite is content-neutral — the resulting tree is byte-identical to the one CI just went green on (git diff between old and new heads is empty). Only the history changed, so the measured numbers in the description still describe exactly this code.

What remains here is the other two ingredients: the NONSTRICT_READ_WRITE reference-metadata bucket and the predefined store-by-reference regions, plus two test updates that this branch necessarily changes what they pin (HibernateEhcacheConfigFileTest now asserts both the inherited and the explicitly-declared heap bound; DataSetMetadataExportServiceQueryCountTest asserts the query count does not grow instead of exact equality).

AI Assisted

[teleivo]

NONSTRICT_READ_WRITE can strand a stale entry for up to 6 hours, affecting each key written independently, not just for a brief post-write window. Worth stating explicitly in the trade-off section, since it is otherwise only derivable from the Hibernate source.

The mechanism: a writer under NONSTRICT_READ_WRITE never installs a cache value, it only removes one, and it does so twice per update. Once at flush, EntityUpdateAction.execute -> cacheUpdate -> update(), per entity and early; once after commit, doAfterTransactionCompletion -> afterUpdate -> unlockItem, per transaction and late. Both resolve to removeFromCache on the same key. Between them the row is written but not committed, so a concurrent reader misses, loads the old committed row, and installs it via putFromLoad. Normally the post-commit evict removes it. But that is a point-in-time deletion, not a barrier, so a reader whose put lands after it leaves the old value with nothing left to remove it:

T1 (writer)                          T2 (reader)
flush: evict-at-flush
                                     get()       -> miss
                                     DB load     -> old committed row
COMMIT
evict-after-commit
                                     putFromLoad -> installs STALE value  <-- survives both evicts

The put succeeds because NONSTRICT_READ_WRITE inherits the unguarded putFromLoad with no version or timestamp check. Previous, READ_WRITE overrides it: lockItem leaves a SoftLockImpl in the slot for the whole window and putFromLoad tests isWriteable against it and returns false, so the late put is refused rather than raced. That override is the entire difference on the read path.

No ordering is guaranteed between the reader's put and the post-commit evict, so this cannot be argued away on probability: it will occur under enough concurrency. No counter distinguishes a stale hit from a valid one, and reproducing it means winning a scheduling race, so it will surface only as an unexplained "metadata edit did not take effect".

With nothing left to remove it, the stale entry survives until the configured 6h ttl, the next write to that key, or a manual evict, and since these entities carry their Sharing JSONB that 6h is also the worst-case ACL-revocation lag.

[teleivo]

How did you come up with the number of entries per metadata?

[jason-p-pickering]

This should be slightly higher and may also require people to use a custom ehcache.xml file, if they have extremely large amounts of orgunits for example. The vast majority of the time, most DBs will not have this many orgunits but there are cases when it exceeds this. All of these seem like reasonable default values, but we should probably have a second look.

[david-mackessy]

The reported performance improvements look good - nice job 👍

Some questions:

1. This change looks like a new global config, is this correct?
2. If it is new global config:
   • have we intentionally opted out from setting this new config globally using hibernate.cache.default_cache_concurrency_strategy ?
   • What is the reason to set it for each individual type/property? (being explicit, easy to see etc.)
   • There is the risk of adding new types/properties and forgetting to add this required behaviour
3. Have some types been explicitly left using read-write? e.g. Program, DataSet and Section (there are probably more)
4. If they have been explicitly left using read-write, what is the reason?

I think the worst case risk (taking into account the low probability) from a stale value needs to be stated very clearly and accepted by Product.

[ameenhere]

The performance improvements are definitely mouth watering. Nice work @netroms
Overall looks good to me considering the benefits seen vs risks stated.

Would it be possible to use our performance test gh pipeline to generate a perf test comparison report (baseline as master and candidate as this PR)?

[netroms]

The reported performance improvements look good - nice job 👍

Some questions:

1. This change looks like a new global config, is this correct?

2. If it is new global config:

   • have we intentionally opted out from setting this new config globally using hibernate.cache.default_cache_concurrency_strategy ?
   • What is the reason to set it for each individual type/property? (being explicit, easy to see etc.)
   • There is the risk of adding new types/properties and forgetting to add this required behaviour

3. Have some types been explicitly left using read-write? e.g. Program, DataSet and Section (there are probably more)

4. If they have been explicitly left using read-write, what is the reason?

I think the worst case risk (taking into account the low probability) from a stale value needs to be stated very clearly and accepted by Product.

@david-mackessy :

1. No, nothing is global in the config. The PR makes 72 explicit per-mapping strategy declarations (hbm usage attributes and @Cache annotations. The ehcache.xml define cache regions for only the ones defined. Everything else is not affected.

2. hibernate.cache.default_cache_concurrency_strategy, is intentionally not used, for two reasons. First, it only applies to cached mappings that do not declare a strategy, every entity in DHIS2 declares one explicitly. Second, we need two strategies to coexist by design. For example Period/RelativePeriods are written at runtime by period generation and must keep READ_WRITE.

3. (4) Yes, deliberately. Program, DataSet, Section, ProgramStage, TrackedEntityType, TrackedEntityAttribute, User, UserRole, UserGroup, Visualization and roughly 50 other mappings remain READ_WRITE, unchanged.
   The rule: only static reference metadata that is read millions of times between writes and that dominated the measured get-counts moved to NONSTRICT_READ_WRITE.

On the risk: A new cached entity that declares READ_WRITE gets the old, safe-but-slower behaviour, never the staleness trade-off silently. Worst case someone copies an existing entity with NONSTRICT_READ_WRITE set, and then creates a new entity with that as a template, without knowing the difference.

[netroms]

The performance improvements are definitely mouth watering. Nice work @netroms Overall looks good to me considering the benefits seen vs risks stated.

Would it be possible to use our performance test gh pipeline to generate a perf test comparison report (baseline as master and candidate as this PR)?

@ameenhere done. Two dispatches of performance-tests.yml on the project's own perf
runner, back to back on the same box, identical inputs except the image: baseline is
dhis2/core-dev:latest (master, d6432be8a194), candidate is dhis2/core-pr:24859,
which is this PR's head 809d8e332d built and published by DHIS2's own CI. Simulation and
harness come from #24767 (refs/pull/24767/head, tip 75cb61fae5), an option-heavy
single event tracker import ramp, 10 to 400 concurrent users, ~130 s per step, Sierra
Leone DB.

Runs, with the full Gatling reports and wall-clock flame graphs as artifacts:

• baseline (master): https://github.com/dhis2/dhis2-core/actions/runs/31624452595
• candidate (this PR): https://github.com/dhis2/dhis2-core/actions/runs/31627426965

Successful imports per second, per concurrency step (an import only counts as OK if the
response body is JSON with status: OK):

step	master ok-rps	master p99	master failed %	#24810 ok-rps	#24810 p99	#24810 failed %
c010	1.25	9.8 s	0%	9.26	1.1 s	0%
c050	1.13	46.6 s	0%	8.06	4.0 s	0%
c100	0.44	58.2 s	41%	7.11	8.6 s	0%
c200	0.38	59.2 s	88%	5.84	21.4 s	0%
c400	0.01	50.9 s (n=1)	99.7%	6.11	35.1 s	0%

Over the whole run master completed 443 imports with a 33.7% error rate across all
requests; the PR completed 4,898 imports with zero errors of any kind in 21,430 requests.
At 400 users master got exactly one import through.

Two things to know when reading the baseline report so nobody misreads it:

1. The error wall that reads jsonPath(...) preparation crashed: ... Unexpected character ('<') is not a harness bug. Under saturation logins start failing with 500s,
   sessionless users get redirected to the login page, and that page answers 200 text/html
   in a few ms. The simulation asserts JSON bodies exactly so those bounces count as
   failures instead of fast successes. The rest of the failures are requests hitting the
   60 s timeout ceiling, which is the collapse itself.
2. The perf runner is weaker hardware than the box behind the numbers in the PR
   description, so the collapse arrives earlier and harder here (master is already
   drowning at 10 users). Compare the ratios and the shape, not the absolutes. Both boxes
   tell the same story: master falls over from ~100 concurrent users on this workload and
   the PR does not.

If you want to run it yourself the exact dispatch recipe (one gh workflow run per arm)
is in the #24767 description. Only org members can dispatch, and run the two arms
serially: the workflow's concurrency group cancels a parallel second run.

[teleivo]

Thank you @netroms for updating the PR and re-running the tests! The numbers do look great 😄 (which is also understandable given we remove locks from the cache algorithm).

Both "rare" and "a few milliseconds" are unmeasured. We don't know the frequency, and the window is not how much code runs between commit and eviction but how long that code takes in wall-clock time. OS scheduling affects that, and the delay grows under exactly the load this PR targets. Stranding is simply a possibility with an unknown rate, which means the decision has to be based on whether the worst case is acceptable at any frequency.

[jason-p-pickering]

Thank you @netroms for updating the PR and re-running the tests! The numbers do look great 😄 (which is also understandable given we remove locks from the cache algorithm).

Both "rare" and "a few milliseconds" are unmeasured. We don't know the frequency, and the window is not how much code runs between commit and eviction but how long that code takes in wall-clock time. OS scheduling affects that, and the delay grows under exactly the load this PR targets. Stranding is simply a possibility with an unknown rate, which means the decision has to be based on whether the worst case is acceptable at any frequency.

Worst case scenario is server crash which we have seen multiple times. The increase RPS here is a great by-product, but that is not the actual point of this PR. It is to prevent the server from locking up.