-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Memory usage when sending the exact same message to multiple clients #537
Comments
Hello @MasterEx, first of all thx for your long and detailed post! :) Let's talk about your proposed changes. That is why 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. Greetings |
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 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! |
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 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. Thank you for your insight! What do you think if I implement something like your Greetings |
Hi @marci4, Yes, I think a " Just a thought, that you may already have in mind, about it. In most cases a 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 Thank you very much for your time and effort! |
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 |
Hi @marci4, Yes, sure! Thanks! |
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. Greetings |
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 inWebSocketImpl
public. Then one can create the message (List<ByteBuffer>
) once andwrite
it to each client. Here is an example: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
methodpublic
. Do you have any suggestions to achieve this in a better way?The text was updated successfully, but these errors were encountered: