HBASE-25998: Redo synchronization in SyncFuture #3382
Merged
Conversation
This file contains 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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
reviewed
Jun 12, 2021
| // Lock protecting the thread safe fields | ||
| ReentrantLock doneLock; | ||
| // Condition to wait on for client threads. | ||
| Condition isDone; |
There was a problem hiding this comment.
nit: isDone is name of a method, not of a data member.
There was a problem hiding this comment.
And name it something else because isDone method already?
|
Looks good. |
bharathv
force-pushed
the
sync_future_fix_sync
branch
2 times, most recently
from
0a970fb
to
f91693d
Compare
bharathv
force-pushed
the
sync_future_fix_sync
branch
from
f91693d
to
caf0f57
Compare
bharathv
changed the title
saintstack
approved these changes
Jun 14, 2021
| @@ -363,7 +362,8 @@ private void syncCompleted(AsyncWriter writer, long processedTxid, long startTim | |||
| // sync futures then just use the default one. | |||
| private boolean isHsync(long beginTxid, long endTxid) { | |||
There was a problem hiding this comment.
Just to note that above the methods are changed from getTxid to getTxId but then here we have beginTxid... super nit... but you might want them to be consistent? Just leave the getTxid?
There was a problem hiding this comment.
The whole Txid change mess was unintentional, undone.
| @@ -1,5 +1,5 @@ | |||
| /** | |||
| * Licensed to the Apache Software Foundation (ASF) under one | |||
| *Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
super super super nit...^ Put space back
|
|
||
| /* | ||
| * Fields below are created once at reset() and accessed without any lock. Should be ok as they | ||
| * are immutable for this instance of sync future until it is reset. |
There was a problem hiding this comment.
Is it worth it doing all this handling just so we reuse SyncFuture instances? Maybe a later experiment would be trying to create a SyncFuture every time? Just a thought.
There was a problem hiding this comment.
Ya, the whole SyncFuture caching seems unnecessary, I was discussing this with my colleagues at work. cc: @apurtell @d-c-manning
I don't have the full historical context here but if I were to guess I think it is because we don't want GC to (over) work with millions of these small sync future objects since they are created once per mutation. May be the latest versions of Java garbage collectors don't need these kind of optimizations, agree that this needs more performance analysis. Let me create a jira to perf analyze the removal of this.
| } | ||
|
|
||
| synchronized boolean isForceSync() { | ||
| boolean isForceSync() { | ||
| return forceSync; |
There was a problem hiding this comment.
This should be covered by the lock? The comment at head of the class says this data member should be covered by the lock? ("all below")
There was a problem hiding this comment.
There is also this comment..
/*
* Fields below are created once at reset() and accessed without any lock. Should be ok as they
* are immutable for this instance of sync future until it is reset.
Apache9
reviewed
Jun 14, 2021
| @@ -132,7 +131,7 @@ | |||
| private static final Logger LOG = LoggerFactory.getLogger(AsyncFSWAL.class); | |||
|
|
|||
| private static final Comparator<SyncFuture> SEQ_COMPARATOR = (o1, o2) -> { | |||
| int c = Long.compare(o1.getTxid(), o2.getTxid()); | |||
| int c = Long.compare(o1.getTxId(), o2.getTxId()); | |||
There was a problem hiding this comment.
Why this change? I think in most places we just use txid instead of txId...
There was a problem hiding this comment.
And maybe for JDK8, we could use
Comparator.comparingLong(SyncFuture::getTxid).thenComparingInt(System::identityHashCode);
There was a problem hiding this comment.
Txid change was unintentional, undone.
Comparator change: Done.
| * The transaction id of this operation, monotonically increases. | ||
| * Lock protecting the thread-safe fields. | ||
| */ | ||
| ReentrantLock doneLock; |
| /** | ||
| * Condition to wait on for client threads. | ||
| */ | ||
| Condition doneCondition; |
| if (t != null && t != Thread.currentThread()) { | ||
| throw new IllegalStateException(); | ||
| } | ||
| t = Thread.currentThread(); | ||
| if (!isDone()) { | ||
| throw new IllegalStateException("" + txid + " " + Thread.currentThread()); | ||
| } | ||
| this.doneLock = new ReentrantLock(); |
There was a problem hiding this comment.
The lock is also reset everytime?
There was a problem hiding this comment.
I was just being extra cautions, just in case there are any dangling locks and such from previous invocation but thinking about it again, that is probably not needed.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Agree its not just synchronized vs j.u.concurrent. It looks like synchronized is a teeny bit slower compared to concurrent implementations under contention (based on benchmarks, example) but in general there shouldn't be a substantial difference, especially with modern JVM versions. I don't think it is biased locking because synchronized inherently supports biased locking (and interestingly it is being removed) and that is not a differentiator. I've looked at the assembly of the JIT-ed code for this function but it was not obvious to me (I'm not an expert at reading x86 assembly, so may I have missed something too). Just looking at the flame graphs with/without change I can see that there is less contention around locks due to a reasons like no busy wait, instead there is a CV wait (so there is no constant monitor contention in a loop until woken up, there is no contention around certain fields like txid, forcesync and probably some help from reentrant lock being faster than object monitors. |
bharathv
force-pushed
the
sync_future_fix_sync
branch
from
caf0f57
to
fd09ca7
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
apurtell
approved these changes
Jun 15, 2021
Apache9
requested changes
Jun 16, 2021
| synchronized boolean isThrowable() { | ||
| return isDone() && getThrowable() != null; | ||
| boolean isDone() { | ||
| if (doneLock == null) { |
There was a problem hiding this comment.
This is a bit confusing... What if later a developer call this method before calling reset? Since now we will not create the lock and condition everytime in reset, let's just create it ion the constructor? So we do not need to test whether it is null any more.
There was a problem hiding this comment.
This is a good point, done
Apache9
reviewed
Jun 16, 2021
| Throwable getThrowable() { | ||
| doneLock.lock(); | ||
| try { | ||
| if (!isDone()) { |
There was a problem hiding this comment.
Better to just test this.doneTxid != NOT_DONE? Or introduce an unlocked isDone version may be called isDoneWithoutLock to call it in both isDone and getThrowable, so we do not need to lock in the isDone again here.
There was a problem hiding this comment.
It is a ReentrantLock, so in the same thread context, it doesn't acquire again?
There was a problem hiding this comment.
Also, if you are saying we can access doneTxid without a lock for specific codepaths, long operations per JVM spec are not atomic since it is split into two 32bit operations, so we may see a corrupt value and hence can result in a wrong isDone() check?
There was a problem hiding this comment.
No, it will be acuired again, as you still need to call unlock one more time you call lock one more time. Reentrant means you can lock the lock multiple times in the same thread, but at least it will waste a bit more cycles.
I mean change the code like this:
boolean isDoneWithoutLock() {
return this.doneTxid != NOT_DONE;
}
boolean isDone() {
doneLock.lock();
try {
return inDoneWithoutLock();
} finally {
doneLock.unlock();
}
}
boolean done() {
doneLock.lock();
try {
if (inDoneWithoutLock()) {
return false;
}
...
} finally {
doneLock.unlock();
}
}
long get() {
doneLock.lock();
try {
while (!inDoneWithoutLock()) {
...
}
...
} finally {
doneLock.unlock();
}
}
There was a problem hiding this comment.
I do not include modifiers here, just an example, not the final code. The isDoneWithoutLock should be private...
There was a problem hiding this comment.
Ok, re-entrant acquisition of the lock should be very low penalty but yes doing this way shaves off a few cycles, will do.
bharathv
force-pushed
the
sync_future_fix_sync
branch
from
fd09ca7
to
0458760
Compare
bharathv
commented
Jun 16, 2021
| Throwable getThrowable() { | ||
| doneLock.lock(); | ||
| try { | ||
| if (!isDone()) { |
There was a problem hiding this comment.
Also, if you are saying we can access doneTxid without a lock for specific codepaths, long operations per JVM spec are not atomic since it is split into two 32bit operations, so we may see a corrupt value and hence can result in a wrong isDone() check?
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Saw your last comment, glad to see there are performance increasing on a real cluster. What I mentioned above is the result for WALPE, I haven't seen you or @apurtell explicitly mentioned the environment, on a HDFS or just a local mini cluster or something else. As the performance number is too good for WALPE, I want to double confirm here. Thanks. |
bharathv
commented
Jun 17, 2021
There was a problem hiding this comment.
I want to double confirm here.
Sure, unfortunately my tooling doesn't support branch-2 yet (WIP), so can't test AsyncWAL on a real distributed HDFS cluster but as I noted the gains reported in the jira are specifically with async wal and not the other implementation.
| Throwable getThrowable() { | ||
| doneLock.lock(); | ||
| try { | ||
| if (!isDone()) { |
There was a problem hiding this comment.
Ok, re-entrant acquisition of the lock should be very low penalty but yes doing this way shaves off a few cycles, will do.
OK, no problem. I think the current information is enough to merge this PR. We could do more test in the future add fill the release note :) |
|
🎊 +1 overall
This message was automatically generated. |
bharathv
force-pushed
the
sync_future_fix_sync
branch
from
0458760
to
255d58e
Compare
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Jun 17, 2021
|
|
||
| private boolean forceSync; | ||
|
|
||
| SyncFuture() { | ||
| this.doneLock = new ReentrantLock(); |
There was a problem hiding this comment.
These two fields could be marked as final if we never change them after construction.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
approved these changes
Jun 17, 2021
| * The transaction id of this operation, monotonically increases. | ||
| * Lock protecting the thread-safe fields. | ||
| */ | ||
| private ReentrantLock doneLock; |
| /** | ||
| * Condition to wait on for client threads. | ||
| */ | ||
| private Condition doneCondition; |
bharathv
force-pushed
the
sync_future_fix_sync
branch
from
255d58e
to
b91c3b0
Compare
Currently uses coarse grained synchronized approach that seems to create a lot of contention. This patch - Uses a reentrant lock instead of synchronized monitor - Switches to a condition variable based waiting rather than busy wait - Removed synchronization for unnecessary fields Signed-off-by: Michael Stack <stack@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
bharathv
force-pushed
the
sync_future_fix_sync
branch
from
b91c3b0
to
6bafb59
Compare
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.
Currently uses coarse grained synchronized approach that seems to
create a lot of contention. This patch