-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Clean up and compress some pre-join packets #15881
base: master
Are you sure you want to change the base?
Conversation
std::istringstream iss(data, std::ios::binary); | ||
std::ostringstream oss(std::ios::binary); | ||
decompressZstd(iss, oss); | ||
data = oss.str(); |
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.
Perhaps it's time for a nice wrapper function? We're effectively copying the data 4 times:
readLongString
(compressed)istringstream
constructor (compressed)is.read(input_buffer, bufsize);
(compressed)os.write(output_buffer, output.pos);
(decompressed)data
assignment (decompressed)
Could be simplified down to:
- (no copy) Read raw data from
NetworkPacket
(compressed) - Write data to an output stream (decompressed)
data
assignment (decompressed)
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.
zero-copy string extraction/input for streams was only invented in C++20: https://en.cppreference.com/w/cpp/io/basic_stringstream/str (or C++26 for string views)
we could implement this ourselves like described here: http://videocortex.io/2017/custom-stream-buffers/
readLongString
could be avoided if the function would return a string_view
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.
See also: #14086
During my research for #15885 I also kept a copy of definitions of every server so I have some nice data for this, too. anyway the important part:
conclusion: (without further tuning) zstd isn't the compression wonder, but it is a speed wonder. 😄 |
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.
Code looks fine. Haven't tested.
Do we maybe want to have a flag per file for whether a media file is compressed? The server could then decide if it wants to compress.
In TOCLIENT_MEDIA
, the names could also be stored separately, and compressed together.
Is the doc in networkprotocol.h supposed to only document the newest protocol version?
Data that zstd doesn't want to compress seems to be passed through with 10 bytes added. It's also still fast at doing this. If speed is really really a problem the Zstd frame format is not complicated and you can just paste the right bytes in front of uncompressed data to turn it into valid zstd.
Yeah, but there isn't that many per bunch actually.
I think so. We have git for the history. |
TOCLIENT_ANNOUNCE_MEDIA:
TOCLIENT_MEDIA:
TOCLIENT_ITEMDEF and NODEDEF:
Why do these matter?
All of them need to happen before the client can join a game session (minus media transfer if cached), and our network layer isn't that good at bulk data transfer.
So if we reduce the time spent here it will lead to faster join times.
What about media compression?
Taking my ~400MB
cache/media
folder I have amassed, zstd takes two(!) seconds to compress it all and manages a reduction to 65% of original size.Zstd will efficiently skip compression if it deems the file to be incompressible.
In case we are ever too concerned about the compression time the server could very well just (pre-)cache all media files.
To do
This PR is Ready for Review.
How to test