-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Prepared message concept #207
Conversation
I took a quick look at this. I might be mistaken, but it looks like PreparedMessage payload fields share memory with pooled connections. The fake connection is clever. I don't think the a massive refactoring is required, but I would need to spend some time on it to know for sure. The NextWriter/Write/Close sequence can be replaced with a call to WriteMessage. Some type assertions can be eliminated by declaring netConn as
|
I noticed that the client case is not handled. Window sizes will not be handled when and if that feature is added. To handle all of the possible message variants, I suggest creating the variants lazily from an uncompressed server message.
The options for NewPreparedMessage should be passed as a struct to make it possible to add more options later:
|
Thanks for looking!
Exactly, need a copy
Looks like |
I specified the compression level in the prepared message options because I think that applications will select the compression level based on the size and type of the message data. I don't think it's something that applications will want to set per connection. The connection API has SetCompressionLevel to match SetWriteDeadline and EnableCompression. If I could design the API from scratch, I'd put the options in a struct:
|
As I think about it more, I don't like that prepared messages in my proposal use SetWriteDeadline and ignores EnableCompression and SetCompressionLevel. These are details that the developer should not need to think about. I think it's simpler to say that the prepared message is sent using the connection's current settings. In that case, compression level should be part of the key. I'd like to find a better name than "prepared" for the feature because the feature is really a cache. Perhaps something like this:
I am not happy with this name either, but I think it's better than PreparedMessage. |
@garyburd I'll try to implement everything you said. What's good in |
Sounds good. Do we agree on the following API?
If you would like, I'll take on refactoring the code to eliminate the need for fake conn. I can do this after you are done with everything else. |
Yes, let me then push everything we discussed and then you can take over on this. |
Oh, well, this won't pass also on old go versions because I use |
c.newCompressionWriter = compressNoContextTakeover | ||
c.SetCompressionLevel(key.compressionLevel) | ||
} | ||
writeErr := c.WriteMessage(pm.frameType, pm.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shadowing here, need writeErr = c.WriteMessage(pm.frameType, pm.data)
@garyburd did you have a chance to turn to this since then? Maybe I can help with sth - at least fix tests and shadowing? |
Let's continue the discussion at #211 |
This is a concept of
PreparedMessage
seen in #182 to improve memory usage with enabled compression mentioned in #203I don't pretend that this pull request will be merged in a form as it's now. It's just an attempt to prove that preparing message can help a lot to reduce CPU and memory usage in PUB/SUB scenarios with many active subscribers (broadcast) especially when permessage compression is enabled.
I wrote several benchmarks inside - create many (10k) connections and send the same message to them:
It's possible to tweak some constants in benchmarks to modify behaviour a bit (num of conns, num of messages on every iteration).
Here results from my machine:
The difference is huge! This makes me feel that I did something wrong:) But at least I wrote tests to compare connection streams when using
PreparedMessage
and not. Also tried on simple chat demo - it works.@garyburd as you can see I make
PreparedMessage
using fake connection writers - it was the simplest solution I could imagine that does not require massive refactoring and a bigger chance for me to make a mistake.