Using BufferedStream over NetworkStream for performance improvement. #1041

navin22 · 2017-09-05T12:34:30Z

Used BufferedStream over NetworkStream while writing the data to reduce the number of calls.
Used BufferedSize of 16KB.
Didn't use BufferedStream while reading, as this didn't add much perf improvements. And was hanging when we were doing Socket.Poll.

- Used BufferedStream over NetworkStream while writing the data to reduce the number of calls. - Used BufferedSize of 16KB.

codito · 2017-09-06T03:28:11Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs

-                this.binaryReader = new BinaryReader(this.stream);
+                this.socket.NoDelay = true;
+                this.stream = new PlatformStream().PlatformBufferedStream(client.GetStream(), SocketConstants.BUFFERSIZE);
+                this.binaryReader = new BinaryReader(client.GetStream());


Is it possible to add a test for this case in Platform Tests suite? It is okay if we add this test only for the newer SocketClient and SocketServer. #Closed

codito · 2017-09-06T03:29:49Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs

@@ -354,8 +355,8 @@ private string TryReceiveRawMessage(CancellationToken cancellationToken)
                        && socketException.SocketErrorCode == SocketError.TimedOut)
                    {
                        EqtTrace.Info(
-                            "SocketCommunicationManager ReceiveMessage: failed to receive message because read timeout {0}",
-                            ioException);
+                        "SocketCommunicationManager ReceiveMessage: failed to receive message because read timeout {0}",


nit: not required change? #Closed

Yeah, added the tabs back. #Closed

codito · 2017-09-06T03:30:40Z

src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IStream.cs

+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces


Suggest Interfaces/IO for this. It is independent of system characteristics. #Closed

codito · 2017-09-06T03:31:54Z

src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IStream.cs

+        /// </summary>
+        /// <param name="stream">Input Stream</param>
+        /// <returns>Buffered Stream</returns>
+        Stream PlatformBufferedStream(Stream stream);


Don't see usage of this API. Can we remove it please? #Closed

codito · 2017-09-06T03:33:38Z

src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IStream.cs

+        /// <param name="stream">Input Stream</param>
+        /// <param name="bufferSize">Buffer Size</param>
+        /// <returns>Buffered Stream</returns>
+        Stream PlatformBufferedStream(Stream stream, int bufferSize);


Could be CreateBufferedStream.

Or we could make it an extension method on Stream. E.g. Stream.ToBufferedStream(int bufferSize). #Closed

harshjain2 · 2017-09-06T06:23:37Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs

                this.binaryWriter = new BinaryWriter(this.stream);

                this.clientConnectedEvent.Set();
-
+                EqtTrace.Info("Using the buffer size of {0} bytes", SocketConstants.BUFFERSIZE);


if(EqtTrace.IsInfoEnabled) { EqtTrace.Info("Using the buffer size of {0} bytes", SocketConstants.BUFFERSIZE); } ``` #Closed

harshjain2 · 2017-09-06T08:01:35Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketClient.cs

@@ -69,7 +70,7 @@ private void OnServerConnected(Task connectAsyncTask)
                throw connectAsyncTask.Exception;
            }

-            this.channel = this.channelFactory(this.tcpClient.GetStream());
+            this.channel = this.channelFactory(new PlatformStream().PlatformBufferedStream(this.tcpClient.GetStream(), SocketConstants.BUFFERSIZE));


Seems like we are using BufferedStream for both Read and Write operations. We need to change it to comply with recommended approach. #Closed

Same applies for SocketServer.cs #Closed

harshjain2 · 2017-09-06T08:06:10Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketClient.cs

@@ -35,7 +36,7 @@ protected SocketClient(Func<Stream, ICommunicationChannel> channelFactory)
            this.cancellation = new CancellationTokenSource();
            this.stopped = false;

-            this.tcpClient = new TcpClient();
+            this.tcpClient = new TcpClient { NoDelay = true };


Using NoDelay = true can be a bit inefficient.
More info here :
https://msdn.microsoft.com/en-us/library/system.net.sockets.tcpclient.nodelay(v=vs.110).aspx #Closed

Yeah, As it also says we need for immediate/important communications.

In reply to: 137197765 [](ancestors = 137197765)

harshjain2 · 2017-09-06T08:08:17Z

test/Microsoft.TestPlatform.PerformanceTests/SocketTests.cs

@@ -6,6 +6,8 @@ namespace Microsoft.TestPlatform.PerformanceTests
    using System;
    using System.Diagnostics;
    using System.Net;
+    using System.Collections.Generic;    


nit : ordering of namespaces. #Closed

smadala · 2017-09-06T08:33:59Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketCommunicationManager.cs

-                this.stream = client.GetStream();
-                this.binaryReader = new BinaryReader(this.stream);
+                this.socket.NoDelay = true;
+                this.stream = new PlatformStream().PlatformBufferedStream(client.GetStream(), SocketConstants.BUFFERSIZE);


Add comment. #Closed

Got it, as we do socket.poll here also. Need to do in Socket.Client and Socket.Server.

In reply to: 137203711 [](ancestors = 137203711)

…into socketbufferfix

codito · 2017-09-07T04:47:40Z

src/Microsoft.TestPlatform.CommunicationUtilities/LengthPrefixCommunicationChannel.cs

@@ -27,7 +27,9 @@ public LengthPrefixCommunicationChannel(Stream stream)
        {
            this.stream = stream;
            this.reader = new BinaryReader(stream, Encoding.UTF8, true);
-            this.writer = new BinaryWriter(stream, Encoding.UTF8, true);
+
+            // Using the Buffered stream while writing.


Good to specify why. #Closed

codito · 2017-09-07T05:00:56Z

test/Microsoft.TestPlatform.PerformanceTests/SocketTests.cs

@@ -4,8 +4,10 @@
 namespace Microsoft.TestPlatform.PerformanceTests
 {
    using System;
+    using System.Collections.Generic;


These changes are not required? #Closed

codito

Looks fantastic. Suggest few nit changes.

IanKemp · 2017-09-07T05:57:15Z

src/Microsoft.TestPlatform.CommunicationUtilities/SocketConstants.cs

+    public class SocketConstants
+    {
+        // Buffer size for the buffered stream we are using.
+        public const int BUFFERSIZE = 16384;


Nit: Isn't the MS standard to use PascalCase for constant names? #Closed

Yeah, Thanks !

In reply to: 137451610 [](ancestors = 137451610)

…into socketbufferfix

Using BufferedStream over NetworkStream for performance improvement.

25aed9b

- Used BufferedStream over NetworkStream while writing the data to reduce the number of calls. - Used BufferedSize of 16KB.

msftclas added the cla-required label Sep 5, 2017

navin22 requested a review from harshjain2 September 5, 2017 12:34

navin22 assigned codito and unassigned codito Sep 5, 2017

navin22 requested review from codito and mayankbansal018 September 5, 2017 12:35

Comments changed & Reverted some changes.

2c902d9

codito reviewed Sep 6, 2017

View reviewed changes

harshjain2 suggested changes Sep 6, 2017

View reviewed changes

Merge branch 'master' into socketbufferfix

9d28a66

smadala reviewed Sep 6, 2017

View reviewed changes

navin22 and others added 5 commits September 6, 2017 14:14

Addressed PR comments.

5e3dd9e

Merge branch 'socketbufferfix' of https://github.com/navin22/nvstest …

4ff6dbe

…into socketbufferfix

Adding test for the hang scenario.

9dc7001

Moving tests from performance folder to platform tests.

5f1d758

Merge branch 'master' into socketbufferfix

bff822f

harshjain2 approved these changes Sep 7, 2017

View reviewed changes

codito reviewed Sep 7, 2017

View reviewed changes

codito approved these changes Sep 7, 2017

View reviewed changes

IanKemp reviewed Sep 7, 2017

View reviewed changes

navin22 added 2 commits September 7, 2017 12:27

Resolved PR comments.

62a1ea9

Merge branch 'socketbufferfix' of https://github.com/navin22/nvstest …

20192a2

…into socketbufferfix

navin22 merged commit cffd8a8 into microsoft:master Sep 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using BufferedStream over NetworkStream for performance improvement. #1041

Using BufferedStream over NetworkStream for performance improvement. #1041

navin22 commented Sep 5, 2017

codito Sep 6, 2017 •

edited by navin22

Loading

codito Sep 6, 2017 •

edited by navin22

Loading

navin22 Sep 6, 2017 •

edited

Loading

codito Sep 6, 2017 •

edited by navin22

Loading

codito Sep 6, 2017 •

edited by navin22

Loading

codito Sep 6, 2017 •

edited by navin22

Loading

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

navin22 Sep 6, 2017

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

smadala Sep 6, 2017 •

edited by navin22

Loading

navin22 Sep 6, 2017

codito Sep 7, 2017 •

edited by navin22

Loading

codito Sep 7, 2017 •

edited by navin22

Loading

codito left a comment •

edited

Loading

IanKemp Sep 7, 2017 •

edited by navin22

Loading

navin22 Sep 7, 2017

Using BufferedStream over NetworkStream for performance improvement. #1041

Using BufferedStream over NetworkStream for performance improvement. #1041

Conversation

navin22 commented Sep 5, 2017

codito Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

codito Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

navin22 Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

codito Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

codito Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

codito Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

harshjain2 Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

harshjain2 Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

harshjain2 Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

harshjain2 Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

navin22 Sep 6, 2017

Choose a reason for hiding this comment

harshjain2 Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

smadala Sep 6, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

navin22 Sep 6, 2017

Choose a reason for hiding this comment

codito Sep 7, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

codito Sep 7, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

codito left a comment • edited Loading

Choose a reason for hiding this comment

IanKemp Sep 7, 2017 • edited by navin22 Loading

Choose a reason for hiding this comment

navin22 Sep 7, 2017

Choose a reason for hiding this comment

codito Sep 6, 2017 •

edited by navin22

Loading

codito Sep 6, 2017 •

edited by navin22

Loading

navin22 Sep 6, 2017 •

edited

Loading

codito Sep 6, 2017 •

edited by navin22

Loading

codito Sep 6, 2017 •

edited by navin22

Loading

codito Sep 6, 2017 •

edited by navin22

Loading

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

harshjain2 Sep 6, 2017 •

edited by navin22

Loading

smadala Sep 6, 2017 •

edited by navin22

Loading

codito Sep 7, 2017 •

edited by navin22

Loading

codito Sep 7, 2017 •

edited by navin22

Loading

codito left a comment •

edited

Loading

IanKemp Sep 7, 2017 •

edited by navin22

Loading