Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IGNITE-23301 Fix scalability issues with coarse locks #4540

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

ascherbakoff
Copy link
Contributor

@ascherbakoff ascherbakoff commented Oct 10, 2024

https://issues.apache.org/jira/browse/IGNITE-23301

Implemented scalable coarse (table) locks.
It only supports IX, S, and SIX lock modes. SIX mode is not avaiable directly, only by upgrading IX->S or S->IX
Short locks are not supported
IX locks are optimized for concurrency.
Implementation is deadlock-free.
Table locks now are partition locks.
IS coarse locks removed because they are not needed.

Benchmark results (Xeon Silver 4314 CPU):
old ver (rev 33c1649)
1 thread
Benchmark (batch) (fsync) (partitionCount) Mode Cnt Score Error Units
UpsertKvBenchmark.upsert 1 false 8 thrpt 20 68650.803 ± 6967.343 ops/s

16 threads
Benchmark (batch) (fsync) (partitionCount) Mode Cnt Score Error Units
UpsertKvBenchmark.upsert 1 false 8 thrpt 20 202157.172 ± 10789.938 ops/s

32 threads
Benchmark (batch) (fsync) (partitionCount) Mode Cnt Score Error Units
UpsertKvBenchmark.upsert 1 false 8 thrpt 20 167636.692 ± 12318.723 ops/s

new ver
1 thread
Benchmark (batch) (fsync) (partitionCount) Mode Cnt Score Error Units
UpsertKvBenchmark.upsert 1 false 8 thrpt 20 74008.938 ± 5447.381 ops/s

16 threads
Benchmark (batch) (fsync) (partitionCount) Mode Cnt Score Error Units
UpsertKvBenchmark.upsert 1 false 8 thrpt 20 290628.268 ± 11657.284 ops/s

32 threads
Benchmark (batch) (fsync) (partitionCount) Mode Cnt Score Error Units
UpsertKvBenchmark.upsert 1 false 8 thrpt 20 320520.489 ± 16084.668 ops/s

@ascherbakoff ascherbakoff changed the title IGNITE-23301 IGNITE-23301 Fix scalability issues with coarse locks Oct 17, 2024
Comment on lines +116 to +117
// .jvmArgsAppend("-Djmh.executor=VIRTUAL")
// .addProfiler(JavaFlightRecorderProfiler.class, "configName=profile.jfc")
Copy link
Contributor

Choose a reason for hiding this comment

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

is it intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, let's keep it.

* {@link ReadWriteLock#readLock()} operations at the cost of {@link ReadWriteLock#writeLock()} operations and memory consumption. It also
* supports reentrancy semantics like {@link ReentrantReadWriteLock}.
*/
public class StripedCompositeReadWriteLock implements ReadWriteLock {
Copy link
Contributor

Choose a reason for hiding this comment

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

You added a new concurrent primitive without any tests, could you pls add some?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆗

Comment on lines 117 to 131
private long p0;

private long p1;

private long p2;

private long p3;

private long p4;

private long p5;

private long p6;

private long p7;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is it for?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've blindly copied it from AI2. Seems it's to prevent false sharing.
But it actually does nothing useful so I've removed it.

assert false : "Unsupported coarse lock mode: " + lockMode;
}

return null; // Should not be here.
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add assertion here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆗


UUID holderTx = slockOwners.keySet().iterator().next();
CompletableFuture<Void> eventResult = fireEvent(LOCK_CONFLICT, new LockEventParameters(txId, holderTx));
if (eventResult.isCompletedExceptionally()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

pls add todo here: // TODO: https://issues.apache.org/jira/browse/IGNITE-21153

Copy link
Contributor Author

@ascherbakoff ascherbakoff Oct 22, 2024

Choose a reason for hiding this comment

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

🆗


break;
default:
assert false : "Unsupported coarse unlock mode: " + lock.lockMode();
Copy link
Contributor

Choose a reason for hiding this comment

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

seems SIX lock mode is also possible here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, in the current implementation SIX lock is acquired by holding two locks: IX + S.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, I should have checked more carefully.

null,
indexIdsAtRwTxBeginTs(txId)
);
if (!IgniteSystemProperties.getBoolean(IgniteSystemProperties.IGNITE_SKIP_STORAGE_UPDATE_IN_BENCHMARK)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's cache this to avoid lookup in System.getProperty()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆗

// Validate reordering with IX locks.
for (Lock lock : ixlockOwners.values()) {
// Allow only high priority transactions to wait.
if (TxIdPriorityComparator.INSTANCE.compare(lock.txId(), txId) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TxIdComparator from deadlockPreventionPolicy should be used here, otherwise we will have lock conflicts if we change the tx id ordering in the deadlock prevention policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆗

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.

4 participants