ibessonov commented on code in PR #4808:
URL: https://github.com/apache/ignite-3/pull/4808#discussion_r1863048074
##########
modules/core/src/main/java/org/apache/ignite/internal/hlc/HybridClockImpl.java:
##########
@@ -111,20 +111,30 @@ public final HybridTimestamp current() {
*/
@Override
public final HybridTimestamp update(HybridTimestamp requestTime) {
- while (true) {
- long now = currentTime();
+ long requestTimeLong = requestTime.longValue();
+ while (true) {
// Read the latest time after accessing UTC time to reduce
contention.
- long oldLatestTime = latestTime;
+ long oldLatestTime = this.latestTime;
- long newLatestTime = max(requestTime.longValue() + 1, max(now,
oldLatestTime + 1));
+ if (oldLatestTime >= requestTimeLong) {
+ return hybridTimestamp(LATEST_TIME.incrementAndGet(this));
+ }
- // TODO https://issues.apache.org/jira/browse/IGNITE-23707 avoid
CAS on logical part update.
+ long now = currentTime();
+
+ 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);
+ if (LATEST_TIME.compareAndSet(this, oldLatestTime,
newLatestTime)) {
Review Comment:
Jira says that we should do an atomic increment in this branch:
> We can skip CAS if only logical part of clocks need to be updated.
This can be achieved using atomic increment, which is more efficient.
I still see a CAS here. Please replace it with `incrementAndGet`, or explain
why can't do this.
Do we have any benchmark results here?
##########
modules/core/src/main/java/org/apache/ignite/internal/hlc/HybridClockImpl.java:
##########
@@ -111,20 +111,30 @@ public final HybridTimestamp current() {
*/
@Override
public final HybridTimestamp update(HybridTimestamp requestTime) {
- while (true) {
- long now = currentTime();
+ long requestTimeLong = requestTime.longValue();
+ while (true) {
// Read the latest time after accessing UTC time to reduce
contention.
Review Comment:
This comment makes no sense now in its current position, please move,
update, or remove it.
Or, please explain why you moved `long now = currentTime();` below this read
despite the presence of this comment. The order of operations was a deliberate
choice by me, and I left this comment to explain the choice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
