HBASE-24962 Optimize BufferNode Lock #2343
Open
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
cuibo01
force-pushed
the
HBASE-24962-master
branch
from
1130b79 to
db49131
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
reviewed
Sep 3, 2020
| @@ -298,6 +299,8 @@ default boolean storeInDispatchedQueue() { | |||
| // ============================================================================================ | |||
| private final class TimeoutExecutorThread extends Thread { | |||
| private final DelayQueue<DelayedWithTimeout> queue = new DelayQueue<DelayedWithTimeout>(); | |||
| private final ConcurrentHashMap<DelayedWithTimeout, DelayedWithTimeout> pendingBufferNode = | |||
There was a problem hiding this comment.
nit: please use ConcurrentMap
There was a problem hiding this comment.
?why use concurrentMap
There was a problem hiding this comment.
As a general rule, it's good to handle implementation by using interface to avoid the pain of changing all references if we wish to change the implementation. I understand, we don't want to change implementation here, but it's always great to refer to implementation using interface unless we want to deliberately use implementor methods which are not overridden from interface.
Hence, just like why we would want to use Map<Key,Val> map=new HashMap<>, let's also use:
private final ConcurrentMap<DelayedWithTimeout, DelayedWithTimeout> pendingBufferNode = new ConcurrentHashMap<>();
There was a problem hiding this comment.
thx @virajjasani
yes, but i think it is okay to use either, RemoteProcedureDispatcher doesn't change often
hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/RemoteProcedureDispatcher.java
Outdated
Show resolved
Hide resolved
hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/RemoteProcedureDispatcher.java
Outdated
Show resolved
Hide resolved
| private final Set<RemoteProcedure> dispatchedOperations = new HashSet<>(); | ||
| private Set<RemoteProcedure> operations = Sets.newConcurrentHashSet(); | ||
| private final Set<RemoteProcedure> dispatchedOperations = Sets.newConcurrentHashSet(); | ||
| private final Object lock = new Object(); |
There was a problem hiding this comment.
While this looks promising but add(RemoteProcedure operation) and abortOperationsInQueue() will now contend with each other. Do we expect both to synchronize on the same object?
There was a problem hiding this comment.
yeah, but abortOperationsInQueue does't happen very often
| ((BufferNode) task).dispatch(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public void putIfAbsent(BufferNode bufferNode) { | ||
| if (pendingBufferNode.putIfAbsent(bufferNode, bufferNode) == null) { |
There was a problem hiding this comment.
If we want to add same object as key and value, why are we using ConcurrentHashMap instead of Sets.newConcurrentHashSet()?
There was a problem hiding this comment.
use atomically putIfAbsent.
| @@ -370,39 +383,47 @@ public TRemote getKey() { | |||
| } | |||
|
|
|||
| @Override | |||
| public synchronized void add(final RemoteProcedure operation) { | |||
| if (this.operations == null) { | |||
There was a problem hiding this comment.
Are you sure we are matching this condition with timeoutExecutor.putIfAbsent(this);?
Based on putIfAbsent() method, it seems only first time when entry is added for new key in map i.e when concurrent map's thread-safe version of putIfAbsent() returns null, we execute this block but is there any scenario that might not adhere to this if (this.operations == null) condition by any chance?
There was a problem hiding this comment.
TimeoutExecutorThread#run will remove task from pendingBufferNode， so after run， task will not be included in the pendingBufferNode
else {
pendingBufferNode.remove(task);
((BufferNode) task).dispatch();
}
| @@ -357,8 +369,9 @@ public void awaitTermination() { | |||
| */ | |||
| protected final class BufferNode extends DelayedContainerWithTimestamp<TRemote> | |||
| implements RemoteNode<TEnv, TRemote> { | |||
| private Set<RemoteProcedure> operations; | |||
| private final Set<RemoteProcedure> dispatchedOperations = new HashSet<>(); | |||
| private Set<RemoteProcedure> operations = Sets.newConcurrentHashSet(); | |||
There was a problem hiding this comment.
RemoteProcedure is parameterized class and it is used in raw format. Although not for this PR, but we should really change this notion of using the class as parameterized in this entire class.
e.g
private Set<RemoteProcedure<TEnv, TRemote>> operations ...
public void add(final RemoteProcedure<TEnv, TRemote> operation) ...
protected abstract void remoteDispatch(TRemote key, Set<RemoteProcedure<TEnv, TRemote>> operations);
Nothing urgent, can be taken up in separate patch to well distinguish the commit purpose.
cuibo01
force-pushed
the
HBASE-24962-master
branch
from
db49131 to
aeed6f5
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
reviewed
Sep 30, 2020
There was a problem hiding this comment.
Do you have numbers to show this a better approach and/or a writeup on the approach and why it better? On cursory glance, we are adding task accounting in a concurrent map -- not cheap -- and then synchronizing on a data member lock instead of synchronizing on the method (though the synchronize on lock extends over the full method call in at least two places so effectively the same price is being paid). Thanks @cuibo01
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
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.
No description provided.