flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] NicoK commented on a change in pull request #6555: [FLINK-10142][network] reduce locking around credit notification
Date Tue, 21 Aug 2018 15:47:13 GMT
NicoK commented on a change in pull request #6555: [FLINK-10142][network] reduce locking around
credit notification
URL: https://github.com/apache/flink/pull/6555#discussion_r211658191
 
 

 ##########
 File path: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/RemoteInputChannel.java
 ##########
 @@ -509,33 +514,40 @@ void onSenderBacklog(int backlog) throws IOException {
 	}
 
 	public void onBuffer(Buffer buffer, int sequenceNumber, int backlog) throws IOException
{
-		boolean success = false;
+		boolean recycleBuffer = true;
 
 		try {
+
+			final boolean wasEmpty;
 			synchronized (receivedBuffers) {
-				if (!isReleased.get()) {
-					if (expectedSequenceNumber == sequenceNumber) {
-						int available = receivedBuffers.size();
+				// Similar to notifyBufferAvailable(), make sure that we never add a buffer
+				// after releaseAllResources() released all buffers from receivedBuffers
+				// (see above for details).
+				if (isReleased.get()) {
+					return;
+				}
 
-						receivedBuffers.add(buffer);
-						expectedSequenceNumber++;
+				if (expectedSequenceNumber != sequenceNumber) {
+					onError(new BufferReorderingException(expectedSequenceNumber, sequenceNumber));
+					return;
+				}
 
-						if (available == 0) {
-							notifyChannelNonEmpty();
-						}
+				wasEmpty = receivedBuffers.isEmpty();
+				receivedBuffers.add(buffer);
+				recycleBuffer = false;
+			}
 
-						success = true;
-					} else {
-						onError(new BufferReorderingException(expectedSequenceNumber, sequenceNumber));
-					}
-				}
+			++expectedSequenceNumber;
+
+			if (wasEmpty) {
+				notifyChannelNonEmpty();
 
 Review comment:
   actually, I didn't measure every single change's performance impact, but having `notifyChannelNonEmpty()`
under the lock would mean that the lock around `RemoteInputChannel#receivedBuffers` would
compete with the lock around `SingleInputGate#inputChannelsWithData`
   -> that change is one of the two lock contention improvements and I'll separate these
into one commit vs. one hotfix for `moreAvailable`

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message