Suggestion for improving OnMessageReceived
Closed this issue · 2 comments
You have code where first we add a message to the stack, then if there is an Action we delete it. This results in unnecessary memory allocation and useless add and delete operations. It's better not to add to the queue unless necessary.
The same code:
private void OnMessageReceived(WebSocketMessage message)
{
if (IsPinging && message.Equals(Config.PingMessage))
return;
_incomingMessages.AddLast(message);
if (MessageReceived == null)
return;
MessageReceived.Invoke(this, message);
_incomingMessages.RemoveLast();
}
Hi. It's not useless because it maintains the API contract in the public IncomingMessages property. Is this why you closed the issue already?
FYI that message list used to be a Queue but I converted it to LinkedList for that specific reason, then just converted _outgoingMessages for consistency. The tradeoff I made was that people were confused that they had to manually remove the messages from the list when using the event hander. I agree it's not memory efficient though, using a List<WebSocketMessage> would achieve that since I'm pretty sure List<T> just changes a size counter under the hood. I'll look into doing that.
Welp, I didn't end up using Lists, but I did push a new backward-incompatible release
- I changed the message buffers back to Queues
- I now do a re-enqueue loop to remove the last element from the incoming Queue when an event handler is installed
This was not my favorite thing to do, but I like this change over other solutions since it keeps the code compact and clean, reduces garbage memory, and it makes more conceptual sense to have the buffers be Queues instead of LinkedLists just to make the last element removal O(1).
I also managed to avoid using a custom collection, but might reconsider doing that if a strong reason to do so presents itself.