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-23303 Improve transaction performance. #4700

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

Conversation

ascherbakoff
Copy link
Contributor

@ascherbakoff ascherbakoff commented Nov 11, 2024

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

Major changes in PR:

  1. Reduced number of clock ticks on txn path
  2. Commit timestamp is propagated to client's observable timestamp for full txn to avoid unnecessary waiting.
  3. Removed System.currentTimeMillis mocks
  4. raft messages don't propagate HLC
  5. Removed some contention points on txn path

Benchmark results (Xeon(R) Silver 4314):
32 threads
Before (rev d6d402d)
Benchmark (batch) (fsync) (partitionCount) Mode Cnt Score Error Units
UpsertKvBenchmark.upsert 1 false 64 thrpt 20 442348.635 ± 14953.282 ops/s

After
Benchmark (batch) (fsync) (partitionCount) Mode Cnt Score Error Units
UpsertKvBenchmark.upsert 1 false 64 thrpt 20 675504.465 ± 40299.612 ops/s

@ascherbakoff ascherbakoff changed the title IGNITE-23303 Improve concurrency for full transactions. IGNITE-23303 Improve transaction performance. Nov 19, 2024

long newLatestTime = max(requestTime.longValue() + 1, max(now, oldLatestTime + 1));

// TODO avoid CAS on lower value.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend rewriting this method in this way (without TODO). It can skip CAS in cases where the letest clock value is the most one and does not notify a listener in case where the clock is chaged without jump up.
@rpuch Could you approve this approach with the listener?

public HybridTimestamp update(HybridTimestamp requestTime) {
        long requestTimeLong = requestTime.longValue();

        while (true) {
            long now = currentTime();

            // Read the latest time after accessing UTC time to reduce contention.
            long oldLatestTime = this.latestTime;

            if (oldLatestTime >= requestTimeLong && oldLatestTime >= now) {
                return hybridTimestamp(LATEST_TIME.incrementAndGet(this));
            }

            if (now > requestTimeLong) {
                if (LATEST_TIME.compareAndSet(this, oldLatestTime, now)) {
                    return hybridTimestamp(now);
                }
            } else {
                long newLatestTime = requestTimeLong + 1;

                if (LATEST_TIME.compareAndSet(this, oldLatestTime, newLatestTime)) {
                    notifyUpdateListeners(newLatestTime);

                    return hybridTimestamp(newLatestTime);
                }
            }
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's do it in separate ticket.
I've created https://issues.apache.org/jira/browse/IGNITE-23707

@sanpwc sanpwc self-requested a review November 20, 2024 11:56
* Tests propagation of HLC on heartbeat request and response.
*/
@Test
public void testHlcPropagation() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

You've left lot's of dead code. E.g. there's no longer need to provide HybridClock to Loza nor store it in NodeOptions. It's just an example. I guess there's more code to remove since we no longer need. Curious why it's not a dedicated PR?

Copy link
Contributor Author

@ascherbakoff ascherbakoff Nov 21, 2024

Choose a reason for hiding this comment

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

The scope of additional changes is small, so I decided to include it.

You've left lot's of dead code

I believe this doesn't break anything.
Created a ticket for further cleaning up:
https://issues.apache.org/jira/browse/IGNITE-23742

Copy link
Contributor

Choose a reason for hiding this comment

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

Ticket description is empty. Anyone who is not in context won't understand what to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the description.

@@ -66,6 +66,13 @@ public class UpsertKvBenchmark extends AbstractMultiNodeBenchmark {
@Param({"8"})
private int partitionCount;

private static final AtomicInteger counter = new AtomicInteger();
Copy link
Contributor

Choose a reason for hiding this comment

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

COUNTER

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆗

@@ -1107,7 +1116,7 @@ public CompletableFuture<Void> upsert(BinaryRowEx row, @Nullable InternalTransac
.transactionId(txo.id())
.enlistmentConsistencyToken(enlistmentConsistencyToken)
.requestType(RW_UPSERT)
.timestamp(clockService.now())
.timestamp(txo.startTimestamp()) // TODO https://issues.apache.org/jira/browse/IGNITE-23712
Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't look right. Every request should send currentTimestamp. Within proposal you will send same time with multiple inserts within same transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intended change and doesn't break txn ordering guaranties.
At txn map phase we need to sync with remote clocks to calculate proper commit timestamp, so sending ts value doesn't matter.
hlc.current can used as well, but it involves more overhead.

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