Skip to content

Replace per-operation backend read lock with a scalable shared-access gate#680

Open
vharseko wants to merge 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/search-1
Open

Replace per-operation backend read lock with a scalable shared-access gate#680
vharseko wants to merge 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/search-1

Conversation

@vharseko

@vharseko vharseko commented Jul 3, 2026

Copy link
Copy Markdown
Member

Summary

Result of profiling the SEARCH hot path with the same methodology as the
rest of the series (#660, #667#670, #672, #674, #678, #679). Every
operation of every type enters the pluggable backend through
BackendImpl.accessBegin() + EntryContainer.sharedLock:

  • one shared AtomicInteger increment/decrement pair
    (threadTotalCount, carrying a long-standing
    FIXME: this is broken comment), and
  • one ReentrantReadWriteLock read acquire/release held for the whole
    operation body.

Read locks do not block each other, but every acquire/release CAS-es the
single AQS state word — at tens of thousands of ops/s across all worker
threads this cache line is the hottest shared write in the server, and
unlike the previous fixes it sits on the path of all operations
(search, bind, compare, modify, add, delete).

Change

The percpu-rwsem pattern, encapsulated in EntryContainer:

  • Hot paths in BackendImpl (10 sites, including hasSubordinates — converted in a review follow-up commit, thanks @maximthomas) register through
    beginSharedAccess()/endSharedAccess(): a striped counter increment
    plus a volatile gate check (CAS + volatile ordering makes the
    Dekker-style store-load pairing safe); they park on a monitor only while
    an exclusive locker has the gate closed. The counter is a same-slot
    StripedCounter, not a LongAdder — see the second review follow-up
    below.
  • Exclusive lockers — index removal, backend configuration changes,
    container close: all rare structural operations — go through the
    existing lock()/unlock() helpers, which now close the gate, drain
    in-flight accesses and still take the RRWL write lock. The write lock
    also keeps excluding the remaining legacy sharedLock readers (export,
    verify, tree listing, entry counting), which stay untouched.
  • threadTotalCount becomes a StripedCounter (only read by
    waitUntilQuiescent() during backend shutdown).

Benchmark

Same harness as the series: packaged server, je backend, 5,000 users,
JDK 11, 8-core host, caffeinate, warm-up run after every restart,
interleaved order old→new→new→old→old→new. Compare load (the highest
operation rate through the backend, hence the most gate traffic):

slot variant ops/s mean p99.9
1 old 39,249 5.09 ms 26.4 ms
2 new 44,342 4.51 ms 18.9 ms
3 new 46,193 4.33 ms 21.8 ms
4 old 29,064 6.87 ms 132.5 ms
5 old 24,992 8.00 ms 134.7 ms
6 new 34,340 5.82 ms 54.2 ms

The shared host fades thermally through the series (visible in both
variants), yet every adjacent pair favors the gate: +13.0% on the
cleanest pair (slots 1↔2) and +37.4% late in the series (slots 5↔6), with
consistently much better tail latency. Errors: 0 in all runs.

Review follow-up (e540fb6)

EntryContainer.lock() no longer abandons the drain loop on interrupt:
returning early could let the exclusive caller (index removal,
configuration change) run concurrently with in-flight shared accesses.
It now records the interrupt, keeps draining until all shared accesses
finish and restores the interrupt status before returning — the same
pattern beginSharedAccess() uses. Covered by a new regression test
testExclusiveLockDrainsSharedAccessDespiteInterrupt in
PluggableBackendImplTestCase: it holds a shared access, interrupts a
thread blocked in lock() and asserts lock() does not return until
endSharedAccess(). Verified both ways: fails deterministically against
the previous behavior, passes with the fix (JETestCase — now 33 tests —
0 failures).

Review follow-up (a9f7ecc)

The drain originally counted in-flight accesses with a LongAdder, which
is not provably safe as a quiescence barrier: LongAdder does not
guarantee that the increment and the backout decrement of one
beginSharedAccess() call land in the same cell (the thread's probe is
rehashed after CAS contention, the cell table grows). A non-atomic
sum() scan may then observe the decrement while missing the paired
increment, under-count and return a false zero while a shared access is
still in flight — letting lock() start the structural operation
concurrently. (An 8-minute targeted stress did not reproduce this on
x86-class hardware, but it is a JMM-permitted execution, and a
correctness barrier needs a provable invariant, not an unobserved race.)

Both counters are now a StripedCounter: a padded AtomicLongArray
whose slot is a pure function of the thread, so the increment and the
decrement of one access are guaranteed to hit the same slot — the
guarantee percpu-rwsem gets from per-CPU counters under
preempt_disable(). A scan reads each slot once, i.e. observes a prefix
of each slot's history, in which a decrement can never appear without
its earlier paired increment; every per-slot subtotal is therefore
non-negative and sum() can only over-estimate, never return a false
zero. The full argument is in the class javadoc.

Testing

mvn -P precommit -pl opendj-server-legacy verify1,443 tests, 0
failures
: AddOperationTestCase (137), CompareOperationTestCase (37),
DeleteOperationTestCase (194), ModifyOperationTestCase (932),
SearchOperationTestCase (75), JETestCase (32 — import/export/verify/
index operations exercising the exclusive↔shared interplay),
OnDiskMergeImporterTest (29), TestImportAndExport (7).

Files

  • opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java
  • opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java
  • opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java
  • opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/StripedCounter.java (new)
  • opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PluggableBackendImplTestCase.java

… gate

Every operation enters the pluggable backend through accessBegin() and
EntryContainer.sharedLock: one AtomicInteger increment/decrement pair
plus a ReentrantReadWriteLock read acquire/release per operation. Read
locks do not block each other, but every acquire/release CAS-es the
single lock state word, and at tens of thousands of operations per
second across all worker threads this cache line becomes the hottest
shared write in the server.

Hot paths in BackendImpl now register through a striped LongAdder gate
(beginSharedAccess/endSharedAccess) and only wait when an exclusive
locker has closed the gate. Exclusive lockers (index removal, backend
configuration changes, close — all rare structural operations) close
the gate via the existing lock()/unlock() helpers, drain in-flight
accesses, and still take the write lock, which also excludes the
remaining legacy sharedLock readers (export, verify, tree listing).
threadTotalCount becomes a LongAdder (read only by waitUntilQuiescent).

Interleaved A/B under the compare benchmark: +13% throughput on the
cleanest adjacent pair (39.2k -> 44.3k ops/s) and consistently better
p99.9 (26 -> 19 ms; 135 -> 54 ms late in the series), every pair
favoring the gate. 1,443 tests pass, including JETestCase and the
import/export suites exercising the exclusive paths.
@vharseko vharseko added this to the 5.2.0 milestone Jul 3, 2026
@vharseko vharseko requested a review from maximthomas July 3, 2026 13:49

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
container.beginSharedAccess();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
container.endSharedAccess();

…gate

Review follow-up: hasSubordinates() still acquired
EntryContainer.sharedLock directly and was missed by the original
conversion because its local variable is named differently. All hot
BackendImpl paths now go through beginSharedAccess()/endSharedAccess().
@vharseko

vharseko commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Good catch — hasSubordinates() was missed by the original conversion (different local variable name). Fixed in b0a4eb5: all hot BackendImpl paths now go through beginSharedAccess()/endSharedAccess().

lock() used to break out of the drain loop on InterruptedException,
allowing the exclusive caller (index removal, configuration change) to
run concurrently with in-flight shared accesses. Defer the interrupt
like beginSharedAccess() does: keep draining and restore the interrupt
status before returning. Add a regression test that fails against the
previous behavior.
@vharseko vharseko requested a review from maximthomas July 4, 2026 06:50
…ped counter

LongAdder gives no guarantee that the increment and the backout decrement
of one beginSharedAccess() call land in the same cell: the probe is
rehashed after CAS contention and the cell table grows. A non-atomic
sum() scan may then observe the decrement while missing the paired
increment, under-count and return a false zero while a shared access is
still in flight, letting lock() start a structural operation
concurrently. Replace both counters (sharedAccessCount, threadTotalCount)
with a striped counter whose slot is a pure function of the thread, so a
scan observes a prefix of each slot's history and can only over-estimate
— the same-slot guarantee percpu-rwsem gets from per-CPU counters.
@vharseko vharseko added the performance Performance / concurrency / lock-contention work label Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmark enhancement performance Performance / concurrency / lock-contention work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants