Reduce per-bind allocations on the bind hot path#672
Open
vharseko wants to merge 2 commits into
Open
Conversation
Two allocation sites ran unconditionally for every bind:
- BindOperationBasis.run() built a CancelRequest and its localized
message even when the bind was the only operation in progress (the
common case on a persistent connection), so nothing could be
cancelled. Build them only when other operations exist.
- PasswordPolicyState.passwordMatches() materialized every stored
password value as a String, split it into String[] components, and
re-encoded the payload back into a ByteString before handing it to
the storage scheme. Parse the {scheme} prefix at the byte level and
pass the payload as a zero-copy sub-sequence instead; malformed
values fall back to the String decoder to preserve error behaviour.
The authPassword syntax path is unchanged.
JFR TLAB profiling under the PR OpenIdentityPlatform#660 benchmark scenario shows the
password-matching share of allocations drop from 3.8% to 2.6% (String
samples -40% at higher throughput); interleaved A/B rounds show
+24..+29% throughput with lower p99.9 on an 8-core host.
This was referenced Jul 3, 2026
maximthomas
approved these changes
Jul 3, 2026
…in progress The size() > 1 guard drew a review concern about a check-then-act window (new operations cannot in fact register while a bind is in progress — both LDAP stacks gate the connection — but the guard's benefit does not justify the debate). CancelRequest and LocalizableMessage are immutable, so a single shared instance restores the unconditional cancelAllOperationsExcept() call while dropping the per-bind allocations entirely — including on binds that do have operations to cancel.
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.
Summary
Allocation-diet follow-up to the bind-path series (#660, #667, #668, #669,
#670): two allocation sites ran unconditionally for every bind, feeding
the GC pressure observed at saturation in #660 ("p99 is GC-bound").
Problem & Change
BindOperationBasis.run()allocated aCancelRequestand itsLocalizableMessageon every bind to cancel outstanding operations —but on a plain re-bind over a persistent connection the bind itself is
the only operation in progress, so there was never anything to cancel.
Both classes are immutable (the message is rendered per locale at use),
so a single shared
static finalinstance is now passed to theunconditional
cancelAllOperationsExcept()call — zero allocations onevery bind, including binds that do have operations to cancel (see the
review follow-up below for the history).
PasswordPolicyState.passwordMatches()processed every storedpassword value as
ByteString → String (whole value) → String[] {scheme, payload} → ByteString (payload re-encoded)before handing it to the storagescheme. The
{scheme}prefix is now parsed at the byte level and thepayload is passed as a zero-copy
subSequence— per stored valuethis removes the full-value
String, onesubstringand theByteString.valueOfUtf8re-encode, leaving only the short lowercasescheme-name string. Malformed values fall back to
UserPasswordSyntax.decodeUserPasswordso error behaviour isbyte-for-byte identical; the authPassword syntax path is unchanged.
Deliberately not included: a shared "anonymous"
AuthenticationInfofor
setUnauthenticated()—AuthenticationInfois mutable(
setMustChangePassword,setAuthenticationDN), so a shared instancewould risk cross-connection corruption.
Benchmark
Same harness as the rest of the series: packaged server,
jebackend,5,000 users (
{SSHA}), JDK 11, 8-core host, both access loggers disabled;authratewith 200 persistent connections re-binding as random users:Interleaved A/B (old → new per round, warm-up run before each measurement,
cool-down before the series):
Allocation attribution (JFR
jdk.ObjectAllocationInNewTLAB, 60 s windowduring steady load): the share of allocation samples in password-matching
stacks drops from 3.8% to 2.6%, with
Stringsamples down 40%(213 → 127) even though the new run processed more binds (47.6k vs 38.6k
total samples). The remaining
byte[]allocations are the Base64 decodeinside the storage scheme itself, out of scope here.
authrateerrors: 0in all runs.
Review follow-up (8938156)
The first version guarded the cancellation with
getOperationsInProgress().size() > 1, which drew a review concern abouta check-then-act window. The race is not actually reachable — both LDAP
stacks gate the connection while a bind is in progress
(
LDAPClientConnection.processDataRead()stops reading the socket andalready-decoded messages are rejected with CONSTRAINT_VIOLATION;
LDAPClientConnection2rejects likewise, andbindInProgressis setbefore the bind operation is enqueued), so the in-progress set can only
shrink between the check and the call. The unconditional old code also
had the identical register-after-cancel window, so the guard changed no
semantics class. Still, the guard's benefit did not justify the debate:
since
CancelRequestandLocalizableMessageare immutable, one sharedstatic final CANCEL_ALL_BY_BIND_REQUESTrestores the unconditional callverbatim and drops the allocations entirely — strictly better than the
guard on both axes.
Testing
mvn -P precommit -pl opendj-server-legacy verifyforBindOperationTestCase(1140),PasswordPolicyTestCase(143),SaltedSHA1PasswordStorageSchemeTestCase(43):1326 tests, 0 failures — re-run with the same result after the
review follow-up.
Files
opendj-server-legacy/src/main/java/org/opends/server/core/BindOperationBasis.javaopendj-server-legacy/src/main/java/org/opends/server/core/PasswordPolicyState.java