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

Made FragmentLength property public #533

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Made FragmentLength property public #533

wants to merge 1 commit into from

Conversation

nvanlaerebeke
Copy link

Made the FragmentLength property public so that someone using the WebSocketSharp library doesn't need to modify & recompile the source.

This is sometimes needed when sending larger amounts of data over the connection, 1kb is too small and has a performance impact.

Ideal is actually 256kb fragments for large transfers, anything higher and there is no real benefit, anything lower takes more CPU & the throughput drops.

@euphoricz
Copy link

euphoricz commented Mar 14, 2019

Yes I completely agree! However I would really like to see this be part of WebSocketBehavior.

An example would be if control information was sent across one socket behavior and images which reflect changes in that control connection could be sent across another socket behavior.

I'm fine with the 1 KB for control messages, however as you said I need a larger buffer / fragmentation length for images, especially if those images are being sent at 15 frames per second. If left at 1 KB too much CPU is consumed.

This is an important distinction between classic cookie cutter websockets which anyone can script and actually being able to use WebSockets in a number of environments while creating well structured separated code.

Another issue with exposing the FragmentationLength is that it is used to create byte arrays, it would be unsafe to change the value of the field after websockets have been created due to race conditions. If the websocket code creates a byte array with a length of FragmentationLength then the software using the lib changes the fragmentationlength, next the websocket send function attempts to perform a stream.read into the byte array at the current fragmentation length it would no longer be the correct number of bytes.

It is unfortunate, FragmentationLength needs to be a read only property of WebSocket (not static), I believe the value for it should be passed into the WebSocket constructor.

@winkmichael
Copy link

winkmichael commented Sep 5, 2019

I'm running into performance issues and I think this is my bottle neck. I guess I will experiment with manually changing this value, but sure would be nice to see it exposed.

Thanks

@euphoricz
Copy link

If you really want to change it I suggest statically changing FragmentLength in WebSocket.cs default constructor to something <= 65536.

Also in the tcplistenerwebsocketcontext websocket constructor add the following at or around line 188.

  if (context.TcpClient != null)
  {
    context.TcpClient.SendBufferSize = 65536;
    context.TcpClient.ReceiveBufferSize = 65536;
  }

It doesn't do any good to increase the fragment size if the send and receive buffers are set to 8192... thus the change above.

@winkmichael
Copy link

Thank you very much for that information. I've been experiementing with the fragment size and not seeing much of a difference in terms of performance, still hitting 100% cpu sending 2 mbps. Hopefully this will help and thanks again it is very much appreciated!

@winkmichael
Copy link

internal WebSocket (TcpListenerWebSocketContext context, string protocol) from Websockets.CS doesn't seem to expose content.TcpClient.SendBufferSize, but I poked around and I see in TcpListenerWebSocketContent.cs internal TcpListenerWebSocketContext (

I've added your suggestion as such
if( tcpClient.Client != null) {
tcpClient.SendBufferSize = 65536;
tcpClient.ReceiveBufferSize = 65536;
}

I am just testing now, and again thanks for your help.

@euphoricz
Copy link

I'm sorry, I forgot to mention that _tcpClient must be exposed in the TcpListenerWebSocketContext.

I added the following property to it:

internal TcpClient TcpClient
{
    get { return _tcpClient; }
}

@winkmichael
Copy link

Thanks, I've been experimenting with that, and trying to up the _fragmentsBuffer.WriteBytes (frame.PayloadData.ApplicationData, 16384); values but I am not seeing any performance improvement at all.

To me it feels like this should have made a pretty big dent in the performance however I am still finding with even just 400 kbps of data being sent in and out, I am at around 75% cpu usage.

I basically am a video broadcaster taking video in from UDP and sending it to all on WS

protected override void OnOpen ()
            {
                if (isFirst == true) {
                    isFirst = false;
                    Console.WriteLine ("First Viewing Session.");
                    while (true) {
                        Sessions.Broadcast (client.Receive (ref localEp));
                    }
}

The first user to view triggers a never ending broadcast event. I've tried doing an async write but that seems to result in data loss.

Thanks again for your time and help

T

@euphoricz
Copy link

That does seem crazy. I pass an mjpeg stream using the same concept except it usually runs at 4 mbps and each individual frame is converted to a base64 string... yet the CPU usage is only between 6 to 10%.

Have you tried running a CPU profiler against it? It would probably help your own sanity if you could nail down exactly where the CPU is being consumed.

Check your receive, make sure you are receiving large chunks of data as well.

How many connected clients are there which you are broadcasting to?

@euphoricz
Copy link

It looks like you are using Socket.Receive, you might make sure you are specifying the amount of data to block for. It could be reading the first few bytes that become available instead of a large chunk of data, that would definitely eat up a ton of CPU.

@euphoricz
Copy link

By the way, if you are still using _tcpClient from WebSocket.cs instead of the _tcpClient from the TcpListenerWebSocketContext it won't work.

From what I can tell _tcpClient in WebSocket.cs is for outgoing websocket connections, it is not set in the constructor so the if block could not be entered and the send / receive buffer size changes would never be set.

For a server side websocket you want to use the tcpclient from the listener context instead.

@winkmichael
Copy link

Thank you for this great information! I've just been learning how to use "The Mono log profiler", so far it is not working ... :) I switched from Udp.Receive to the generic Socket method, as a quick google of your hint regarding Udp.Receive potentially only getting a few bytes and returning seems like a common issue people run into. Despite the change (byte [] msg = new Byte [32768]; s.ReceiveFrom (msg, SocketFlags.None, ref senderRemote);) , I still seem to have a performance bottle neck, but at least I get to learn how to run a profiler on Linux :)

@euphoricz
Copy link

Try using this Receive overload instead...

s.ReceiveFrom(msg, 32768, SocketFlags.None, ref senderRemote);

According to MSDN that forces it to read the specified number of bytes.

ReceiveFrom(Byte[], Int32, SocketFlags, EndPoint) | Receives the specified number of bytes into the data buffer, using the specified SocketFlags, and stores the endpoint.

https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.socket.receivefrom?view=netframework-4.8

@euphoricz
Copy link

You probably already know this but also remember the function returns the actual number of bytes it read, this should be used when working with the array as there could be much less actual data in it or data of value that is.

@euphoricz
Copy link

Nice work with the mono profiler, I still have yet to give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants