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
Add: Support Zstandard(zstd) savegame compression #8773
Conversation
One could imagine having different defaults for on-disk saves and for network-transfer saves. |
eba9544
to
6f0aeaf
Compare
Ran the benchmark for all public server (180) saves. Interestingly on this dataset zstd:9 approaches lzma:2 savegame sizes really close while still being significantly faster. zstd:1 is still the best for network though.
|
I was a bit confused at first looking through this as the the methods used seemed to be mismatched relative to the documentation. For the decompresser, it seems that the method names in the documentation are: ZSTD_createDStream, ZSTD_freeDStream, ZSTD_decompressStream. Likewise for the compressor. The code here seems to mix and match old and new names. |
@JGRennison where did you find ZSTD_createDCtx and ZSTD_freeDCtx being called deprecated? And in general, I've been mostly following those examples as documentation itself is kinda crappy |
I think you're right and I've misunderstood it. I first looked at the documentation from here: https://facebook.github.io/zstd/zstd_manual.html, which implies that the two sets of functions do different things. This issue seems to explain it: facebook/zstd#1510 |
I updated workflows in master to solve the cache issue. You'll need to rebase and increase the number at the end of I still need to find why CMake doesn't find zstd on windows (and it's probably also related to macos failure) |
Cmake should probably be set to only accept version 1.4 or later, as compilation fails on some of the CI release targets, which have 1.3.x versions. |
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.
Given this requires zstd 1.4+, that would mean that accepting this PR would also effectively remove Ubuntu 18.04 and Debian Buster release targets (and I think that should even be part of this PR, in a different commit). As of course it would be rather silly if we distribute binaries where it is not possible to join a multiplayer server with :D
To be clear, I have no opinion about this, I am merely stating what it would effectively mean :)
{"zstd", TO_BE32X('OTTS'), CreateLoadFilter<ZSTDLoadFilter>, CreateSaveFilter<ZSTDSaveFilter>, 0, 101, 122}, | ||
#else | ||
{"zstd", TO_BE32X('OTTS'), nullptr, nullptr, 0, 0, 0}, | ||
#endif |
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.
If I remember correctly, putting it here doesn't make it a default for anyone. Would it be wise to add code that depending on network-save / local-save it picks zstd? As just adding it for the sake of adding it feels a bit mute to me. Opinions? Ideas?
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.
Well, my idea was to just add it first and decide what to do with it later (1.12?) so there is enough time for it to be tested by those who wish to.
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.
Personally not a big fan of using upstream for this, but that is just my take in this :) Mainly because I also think that adding it isn't a lot of work, and would benefit many people :) (in the end, we know it is faster for sure, and sizes on network games do not matter that much after all). But again, just my take on it, curious if others agree/disagree :)
* a good choice for multiplayer servers. And zstd level 1 seems to be the optimal one for client connection speed | ||
* (compress + 10 MB/s download + decompress time), about 3x faster than lzma:2 and 1.5x than zlib:2 and lzo. | ||
* As zstd has negative compression levels the values were increased by 100 moving zstd level range -100..22 into | ||
* openttd 0..122. Also note that value 100 mathes zstd level 0 which is a special value for default level 3 (openttd 103) */ |
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.
This might be very unclear to people that use the config file setting for this. How are we going to make sure it is clear to them that this is happening?
I wonder if we wouldn't be better off making the parameter for compression level signed instead. That avoids this hack, and makes the configuration a bit more understandable for the average user?
Last information I have, is that zstd in fact did not help for savegame filesize. Is this true? In that case, are we better off closing this PR? It seems nobody took an interest in all these years? (open question, I don't mind if the answer is: nah, let's keep it open. As long as we have a reason for doing so :) ) Tnx! |
Well, there is a bit of a chicken-egg problem. My testing ended up being inconclusive, need to try it in real conditions, collecting stats for player connections but that can't be done without implementing it first. Maybe I'll get around to it eventually and do some A/B testing with cmclient or smth. But for now you can close this PR if you want, I can always (ask to) reopen it later. |
Okay, let's close it (and the network-handshake-for-compression) PR for now, but we are always interested in making savegames smaller, so if you can find the time, it would be very welcome. |
Motivation / Problem
Zstd offers a blazingly fast compression/decompression while keeping a reasonable compression ratio. Making it an excellent option for a network server.
Description
For an estimated client connection time on 10 MB/s (compress + download + decompress) it is about 3x faster than lzma:2(default) and 1.5x than zlib:2 and lzo (next best choices). For some cases (e.g. 4kx4k map + no pause on join) it's basically a lifesaver. Yet I didn't make it a default compression since it produces about 25-40% large saves than lzma:2 making it a poor choice for singleplayer. Also, it's a new addition and would be bad if some unexpected issue were to appear for a default compression method.
To compare the performance I did a completely scientific benchmark in python on totally unbiased corpus of 516 savegames in my OpenTTD folder:
Interestingly lz4 doesn't seem to be of any use in the OpenTTD case.
Benchmark script: slcompressioncmp.zip
Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.