Skip to content

IGNITE-28304 Improve contention on waiters in lock manager#7970

Open
Cyrill wants to merge 4 commits intoapache:mainfrom
unisonteam:IGNITE-28304
Open

IGNITE-28304 Improve contention on waiters in lock manager#7970
Cyrill wants to merge 4 commits intoapache:mainfrom
unisonteam:IGNITE-28304

Conversation

@Cyrill
Copy link
Copy Markdown
Contributor

@Cyrill Cyrill commented Apr 10, 2026

expected[w.lockMode.ordinal()]++;
}
}
assert Arrays.equals(expected, lockModeHeldCount)
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.

As I remember, we didn't want to use assert where it's not necessary, is it still so?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed

Comment on lines +956 to +960
for (WaiterImpl w : waiters.values()) {
if (w.lockMode != null) {
expected[w.lockMode.ordinal()]++;
}
}
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.

Are you sure we want to check this every time?
Did you run benchmarks?

Copy link
Copy Markdown
Contributor Author

@Cyrill Cyrill Apr 20, 2026

Choose a reason for hiding this comment

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

Benchmarks (LockManagerBenchmark, 1 fork, default JMH settings):

bench main this PR Δ
contendedSharedKeyS (16 threads, shared key, S-mode) 6.100 ± 0.271 µs/op 4.809 ± 0.362 µs/op −21%
lockCommit (1 thread, 200 unique X keys) 22.757 µs/op 19.022 µs/op −16%


// Fast path: no incompatible holders — grant immediately.
// Conservative for upgrades: the waiter's own mode is in the counts, causing a fallthrough to the slow path.
if (!hasAnyIncompatibleHolder(intendedLockMode)) {
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.

is it correct that the main idea is adding fast path for isWaiterReadyToNotify to reduce the time which tread spends inside synchronized?

Copy link
Copy Markdown
Contributor Author

@Cyrill Cyrill Apr 20, 2026

Choose a reason for hiding this comment

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

yes. More precisely: replace O(N) iteration with O(1) inside the synchronized block

track(waiter.txId, this);
}

assertLockModeHeldCount();
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.

release flow seems to stay uncovered by this fix. Meanwhile, there is also a usage of heavy method under lock: unlockCompatibleWaiters

Copy link
Copy Markdown
Contributor Author

@Cyrill Cyrill Apr 20, 2026

Choose a reason for hiding this comment

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

Checked with a dedicated mixed X/S bench (releaseWakeupMixed: 1 X-writer + 15 S-readers on a shared key, NO_OP policy so S-readers actually queue behind the X-holder). In this setup every X release triggers unlockCompatibleWaiters over a batch of pending S waiters

bench main this PR Δ
releaseWakeupMixed 4.169 ± 0.691 µs/op 3.354 ± 0.295 µs/op −20%
contendedSharedKeyS (16-thread S-only, acquire-side fast path) 6.100 ± 0.271 µs/op 4.809 ± 0.362 µs/op −21%
lockCommit (1-thread, 200 unique X keys — sanity) 22.757 µs/op 19.022 µs/op −16%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants