Memory usage when sending the exact same message to multiple clients

[MasterEx]

Hello,

I was working on a server that has to send the same message to all the connected clients, something like a broadcast.

I've noticed that the memory usage was going high very quickly and the garbage collection was happening too often.

Examining the code shows that in order to send a single byte array, a ByteBuffer object, a BinaryFrame, a List, an ArrayList and at least a new ByteBuffer is created.

So, in this case where one may want to send the exact same content to many clients I guess that it is better not to recreate the objects with the same content for each recipient.

A simple solution that I have in mind is making the write() method in WebSocketImpl public. Then one can create the message (List<ByteBuffer>) once and write it to each client. Here is an example:

public void broadcast(byte[] buffer, Collection<WebSocket> clients) {
        if (!clients.isEmpty()) {
            Draft draft = clients.iterator().next().getDraft();
            List<Framedata> frames = draft.createFrames(ByteBuffer.wrap(buffer), true);
            List<ByteBuffer> outgoingFrames = new ArrayList<ByteBuffer>();
            for (Framedata f : frames) {
                outgoingFrames.add(draft.createBinaryFrame(f));
            }
            for (WebSocket client : clients) {
                client.write(outgoingFrames);
            }
        }
    }

I have tested this thought and by profiling a test application it seems to works ok.

Do you find this approach ok? I don't fill comfortable making a private method public. Do you have any suggestions to achieve this in a better way?

[marci4]

Hello @MasterEx,

first of all thx for your long and detailed post! :)

Let's talk about your proposed changes.
Your solution would work right now but it will probably won't work in the future any more.
The reason for this is that I added the support for websocket extensions with the upcoming release of 1.3.5 (see #463).
Even though right now all the websocket endpoints always use Draft_6455, the resulting binary frame may vary with the new release due to different negotiated compression extensions.

That is why outgoingFrames may not be the same for two different endpoints and for that we need to call draft.createBinaryFrame(f).

What you can do anyways is to create one frame for all the endpoints you want to send it to.

Something like that should work without a problem and would also reduce the recreation of objects in comparison to the current situation.

    public void broadcast(byte[] buffer, Collection<WebSocket> clients) {
        if (!clients.isEmpty()) {
            Draft draft = clients.iterator().next().getDraft();
            Framedata frame = draft.createFrames(ByteBuffer.wrap(buffer), true).get(0); //Right now createFrames for a draft will always return a list with one element.
            for (WebSocket client : clients) {
                client.send(frame);
            }
        }
    }

Hope this helps you a bit.
Apart from this, could you maybe tell me a bit more about the project and why the amount of GC calls is relevant for you.

Greetings
marci4

[MasterEx]

Hi @marci4,

Thank you for your answer. I see how the compression extensions complicate this issue.

I guess this could get partially addressed if the Drafts (or better some subclass of WebSocketImpl, i.e. CachedWebSocketImp) were caching the last X binary frames and reused them whenever this was possible. I am not sure if this is the best idea though.

Regarding your code snippet I guess you wanted to use sendFrame() instead of send() or maybe pass the whole List of FrameData that createFrames returns in send. I guess either would be an improvement.

Based on my original code snippet and your remark that different endpoints may use different Draft implementations I guess this could be an improved solution for many clients.

public void broadcast(byte[] buffer, Collection<WebSocket> clients) {
	Map<Draft, List<ByteBuffer>> draftFrames = new HashMap<>();
	for (WebSocket client : clients) {
		Draft draft = client.getDraft();
		if(!draftFrames.contains(draft)) {
			List<Framedata> frames = draft.createFrames(ByteBuffer.wrap(buffer), true);
			List<ByteBuffer> outgoingFrames = new ArrayList<ByteBuffer>();
			for (Framedata f : frames) {
				outgoingFrames.add(draft.createBinaryFrame(f));
			}
			draftFrames.put(draft, outgoingFrames);
		}
		client.write(draftFrames.get(draft));
	}
}

Regarding the project I am working on, I would like to run a server on an android device and stream data to connected clients (in a local network). As I was stress testing my prototype I saw that the server was making some tiny pauses while sending the data as the number of clients were increasing. The pauses were more obvious after a while.

By running a profiler I identified that the pauses where happening when the garbage collector was running. Of course the garbage collection was happening more often as the memory consumption was getting increased after the addition of each client.

Anyway, thank you for your time and your suggestions!

[marci4]

Hello @MasterEx,

first of all sorry for my delayed answer. Had to finish some stuff for my holiday :)

Caching internally in a draft would not solve this problem, since every connection has it's own instance of a draft. So some subclass of WebSocketImpl would be the better approach.

Yeah, you're right, my bad :( It is sendFrame(). Also thinking about making a public endpoint for sending multiple frames.

You're code snippet would not work, since currently two drafts with the same properties would have separate hashcodes, since we are not overriding the hashCode function right now.
I will look into this and probably change this, if there are no issues with that.

Thank you for your insight!

What do you think if I implement something like your broadcast() method for a String and a byte array. Would that cover every use case for you? If so, I think it would also be a benefit for other users!

Greetings
marci4

[MasterEx]

Hi @marci4,

Yes, I think a "broadcast()" method would be very useful and it would definetely cover my case.

Just a thought, that you may already have in mind, about it. In most cases a broadcast() that sends a String or byte array to all the connected clients would suffice. However, there are cases where one would like to send the same message only to a specific sub-group of the connected clients and not to all.

An example would be a chat application that supports different channels. When a client sends a message to a specific channel then all the other clients connected to the specific channel should receive the message and not all the clients connected to the server.

So in case you implement this, I think that it would be more beneficial if you provide a broadcast() that accepts the list of the clients that the message should be delivered or even better provide two, one that accepts the list of the recipient clients and one that sends the message to all the connected clients.

Thank you very much for your time and effort!

[marci4]

Hello @MasterEx,

yes I thought about implementing it this way!

Opened a separate issue for managing some changes which should improve the total memory usage.

If it is fine with you, I would like to close this issue!

Greetings
marci4

[MasterEx]

Hi @marci4,

Yes, sure! Thanks!

[marci4]

Hello @MasterEx,

sorry that I post in this closed issue again but I want to clear up why I have not implemented it the way you proposed.

The simple reason for that is that a ByteBuffer is not thread-safe.
That could cause the theoretical problem, that the same ByteBuffer will be accessed by two different threads to write to the specific channel and the update of the written position could cause problems e.g. some bytes could not be written.
That is why I still create a separate ByteBuffer for each connection to not get into this situation.

Greetings
marci4