Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using BufferedStream over NetworkStream for performance improvement. #1041

Merged
merged 10 commits into from
Sep 7, 2017
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities
using System.Threading.Tasks;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions;
using Microsoft.VisualStudio.TestPlatform.Utilities;

/// <summary>
Expand All @@ -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 };
Copy link
Contributor

@harshjain2 harshjain2 Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

this.channelFactory = channelFactory;
}

Expand Down Expand Up @@ -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));
Copy link
Contributor

@harshjain2 harshjain2 Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@harshjain2 harshjain2 Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies for SocketServer.cs #Closed

if (this.ServerConnected != null)
{
this.ServerConnected.SafeInvoke(this, new ConnectedEventArgs(this.channel), "SocketClient: ServerConnected");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities
using System.Net.Sockets;
using System.Threading;
using System.Threading.Tasks;

using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions;

/// <summary>
/// Facilitates communication using sockets
Expand Down Expand Up @@ -73,7 +73,7 @@ public class SocketCommunicationManager : ICommunicationManager
/// <summary>
/// Stream to use read timeout
/// </summary>
private NetworkStream stream;
private Stream stream;

private Socket socket;

Expand All @@ -100,7 +100,6 @@ internal SocketCommunicationManager(IDataSerializer dataSerializer)
public IPEndPoint HostServer(IPEndPoint endpoint)
{
this.tcpListener = new TcpListener(endpoint);

this.tcpListener.Start();
EqtTrace.Info("Listening on Endpoint : {0}", (IPEndPoint)this.tcpListener.LocalEndpoint);

Expand All @@ -119,12 +118,13 @@ public async Task AcceptClientAsync()

var client = await this.tcpListener.AcceptTcpClientAsync();
this.socket = client.Client;
this.stream = client.GetStream();
this.binaryReader = new BinaryReader(this.stream);
this.socket.NoDelay = true;
this.stream = new PlatformStream().PlatformBufferedStream(client.GetStream(), SocketConstants.BUFFERSIZE);
Copy link
Contributor

@smadala smadala Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

this.binaryReader = new BinaryReader(client.GetStream());
Copy link
Contributor

@codito codito Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

this.clientConnectedEvent.Set();

EqtTrace.Info("Using the buffer size of {0} bytes", SocketConstants.BUFFERSIZE);
Copy link
Contributor

@harshjain2 harshjain2 Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

EqtTrace.Info("Accepted Client request and set the flag");
}
}
Expand Down Expand Up @@ -165,7 +165,7 @@ public async Task SetupClientAsync(IPEndPoint endpoint)
// for now added a check for validation of this.tcpclient
this.clientConnectionAcceptedEvent.Reset();
EqtTrace.Info("Trying to connect to server on socket : {0} ", endpoint);
this.tcpClient = new TcpClient();
this.tcpClient = new TcpClient { NoDelay = true };
this.socket = this.tcpClient.Client;

Stopwatch watch = new Stopwatch();
Expand All @@ -178,10 +178,11 @@ public async Task SetupClientAsync(IPEndPoint endpoint)

if (this.tcpClient.Connected)
{
this.stream = this.tcpClient.GetStream();
this.binaryReader = new BinaryReader(this.stream);
this.stream = new PlatformStream().PlatformBufferedStream(this.tcpClient.GetStream(), SocketConstants.BUFFERSIZE);
this.binaryReader = new BinaryReader(this.tcpClient.GetStream());
this.binaryWriter = new BinaryWriter(this.stream);
EqtTrace.Info("Connected to the server successfully ");
EqtTrace.Info("Using the buffer size of {0} bytes", SocketConstants.BUFFERSIZE);
this.clientConnectionAcceptedEvent.Set();
}
}
Expand Down Expand Up @@ -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}",
Copy link
Contributor

@codito codito Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not required change? #Closed

Copy link
Member Author

@navin22 navin22 Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, added the tabs back. #Closed

ioException);
}
else
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// 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.CommunicationUtilities
{
public class SocketConstants
{
// Buffer size for the buffered stream we are using.
public const int BUFFERSIZE = 16384;
Copy link

@IanKemp IanKemp Sep 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Thanks !


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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities
using System.Net.Sockets;
using System.Threading;
using System.Threading.Tasks;

using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions;
using Microsoft.VisualStudio.TestPlatform.Utilities;

/// <summary>
Expand Down Expand Up @@ -88,10 +88,11 @@ public void Stop()
private void OnClientConnected(TcpClient client)
{
this.tcpClient = client;
this.tcpClient.Client.NoDelay = true;

if (this.ClientConnected != null)
{
this.channel = this.channelFactory(client.GetStream());
this.channel = this.channelFactory(new PlatformStream().PlatformBufferedStream(this.tcpClient.GetStream(), SocketConstants.BUFFERSIZE));
this.ClientConnected.SafeInvoke(this, new ConnectedEventArgs(this.channel), "SocketServer: ClientConnected");

// Start the message loop
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// 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
Copy link
Contributor

@codito codito Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
using System.IO;

/// <summary>
/// Helper class to return plaform specific stream.
/// </summary>
public interface IStream
{
/// <summary>
/// Returns plarform specific Buffered Stream
/// </summary>
/// <param name="stream">Input Stream</param>
/// <returns>Buffered Stream</returns>
Stream PlatformBufferedStream(Stream stream);
Copy link
Contributor

@codito codito Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// <summary>
/// Returns platrform specific Buffered Stream with desired buffer size.
/// </summary>
/// <param name="stream">Input Stream</param>
/// <param name="bufferSize">Buffer Size</param>
/// <returns>Buffered Stream</returns>
Stream PlatformBufferedStream(Stream stream, int bufferSize);
Copy link
Contributor

@codito codito Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be CreateBufferedStream.

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

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// 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
{
using System.IO;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces;

/// <inheritdoc/>
public class PlatformStream : IStream
{
public Stream PlatformBufferedStream(Stream stream)
{
return new BufferedStream(stream);
}

/// <inheritdoc/>
public Stream PlatformBufferedStream(Stream stream, int bufferSize)
{
return new BufferedStream(stream, bufferSize);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// 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
{
using System;
using System.IO;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces;

/// <inheritdoc/>
public class PlatformStream : IStream
{
public Stream PlatformBufferedStream(Stream stream)
{
throw new NotImplementedException();
}

/// <inheritdoc/>
public Stream PlatformBufferedStream(Stream stream, int bufferSize)
{
throw new NotImplementedException();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// 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
{
using System.IO;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces;

/// <inheritdoc/>
public class PlatformStream : IStream
{
/// <inheritdoc/>
public Stream PlatformBufferedStream(Stream stream)
{
return stream;
}

/// <inheritdoc/>
public Stream PlatformBufferedStream(Stream stream, int bufferSize)
{
return stream;
}
}
}
2 changes: 2 additions & 0 deletions test/Microsoft.TestPlatform.PerformanceTests/SocketTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ namespace Microsoft.TestPlatform.PerformanceTests
using System;
using System.Diagnostics;
using System.Net;
using System.Collections.Generic;
Copy link
Contributor

@harshjain2 harshjain2 Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit : ordering of namespaces. #Closed

using System.Text;
using System.Threading;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
Expand Down