Skip to content

Avoid global write lock on every delete in GroupManager#682

Open
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:features/adddel-1
Open

Avoid global write lock on every delete in GroupManager#682
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:features/adddel-1

Conversation

@vharseko

@vharseko vharseko commented Jul 3, 2026

Copy link
Copy Markdown
Member

Summary

Found while profiling the ADD/DELETE hot paths with the series methodology
(#660, #667#670, #672, #674, #678#681). GroupManager registers an
internal POST_OPERATION_DELETE hook that runs for every delete and
unconditionally takes the manager's global write lock just to call
removeSubtree() on the group-instances map:

lock.writeLock().lock();
try {
  if (groupInstances.removeSubtree(entry.getName(), null)) { refreshToken++; }
} finally { lock.writeLock().unlock(); }

On most servers that map is empty or tiny, and almost no deleted entry has
a group registered at or below it — yet all concurrent delete threads
serialize on this one exclusive lock. The neighbouring
SubentryManager.doPostDelete() already avoids exactly this with a
read-locked fast path.

Change

Add the same fast path to GroupManager.doPostDelete(): check
groupInstances.containsSubtree(dn) under the read lock (an O(1)
hierarchical containsKeyDITCacheMap maintains ancestor nodes, the
same semantics AuthenticatedUsers relies on) and return before touching
the write lock. When a group actually exists at or below the deleted DN,
the original write-locked removeSubtree() still runs. No new races: the
snapshot check is equivalent to the write-locked removal finding nothing.

Honest performance note

An interleaved A/B (old→new→new→old→old→new, addrate -C random mixed
add+delete, 200 persistent connections, same harness as the series) shows
no measurable throughput difference — the cleanest late pair is
1,428 vs 1,454 ops/s (+1.8%), within host noise. The removed critical
section is nanoseconds (HashMap.remove on an empty map) while the delete
path itself is dominated by JE record-lock waits on shared index keys
(~21,700 LockManager.waitForLock events / 822 s per 80 s JFR window in
the baseline profile). This is a structural fix — an unconditional
global exclusive lock has no place on the per-delete path, and it becomes
a real ceiling on wide machines with high deletion rates — not a claimed
local win.

Testing

mvn -P precommit -pl opendj-server-legacy verify:
GroupManagerTestCase (25 — group registration/deregistration, i.e. the
slow path behind the new fast path), IsMemberOfVirtualAttributeProviderTestCase
(47), VirtualStaticGroupTestCase (14), DeleteOperationTestCase (194),
AddOperationTestCase (137): 417 tests, 0 failures.

Files

  • opendj-server-legacy/src/main/java/org/opends/server/core/GroupManager.java

GroupManager.doPostDelete() runs for every delete operation and
unconditionally took the manager's global write lock just to call
removeSubtree() on the group instances map — which is empty or tiny on
most servers, and almost never contains anything at or below the
deleted entry. This serialized all concurrent delete threads on one
exclusive lock.

Add a read-lock fast path that checks containsSubtree() (an O(1)
hierarchical containsKey) and returns before taking the write lock,
mirroring the pattern already used by SubentryManager.doPostDelete().
The write-locked removal still runs whenever a group is actually
registered at or below the deleted DN.

An interleaved A/B under concurrent add+delete load shows no measurable
throughput change on an 8-core host: the critical section was tiny and
the delete path is dominated by JE record-lock waits on shared index
keys. This is a structural fix — removing an unconditional global
exclusive lock from the delete path — rather than a measured win.
@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

enhancement performance Performance / concurrency / lock-contention work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants