Skip to content
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

Feature: Allow client and server to negotiate on compression to use for the savegame #8805

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ldpl
Copy link
Contributor

@ldpl ldpl commented Mar 4, 2021

Motivation / Problem

Client and server can be compiled with different sets of compression libraries. If client doesn't have the compression server uses it just fails. Also game uses lzma:2 for everything which isn't the best option for the network.

Description

Client on join sends bitset of all supported compressions to the server and lets the server to chose whichever it likes more. For that server has network.savegame_formats setting that allows specifying multiple compressions (space-separated) that will be tried in that order (if not specified it uses "zlib:2 lzma:0 lzo:0"). This also allows the server to use different compression methods for network connections and local saves.
In a way this is also a preparation for zstd inclusion (#8773) as otherwise it will have to use the older zstd version to support old platforms.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

src/table/settings.ini Outdated Show resolved Hide resolved
@ldpl ldpl force-pushed the compression-handshake branch from 217de99 to e0b1d88 Compare Mar 4, 2021
@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Mar 4, 2021

More generally, is it really necessary that the server operator is able to manually specify the algorithm order and levels?
Simply using the "correct" values for the current circumstances instead of allowing the user to specify an alternative set would probably make this simpler.
I can't imagine that the existing compression configuration field sees much, if any, use outside of testing.

@James103
Copy link
Contributor

@James103 James103 commented Mar 4, 2021

@JGRennison Some people still prefer to use a slightly larger but much faster savegame compression method (such as lzo instead of lzma) in order to reduce the CPU time required to save the game at the cost of some storage space.

Example: savegame_format = lzo can be up to 14x faster at the cost of only 1.75x the file size (as compared to zlib:6).

@ldpl
Copy link
Contributor Author

@ldpl ldpl commented Mar 4, 2021

@JGRennison My main reason was to allow disabling certain method if something goes wrong with it. Also wouldn't hurt for better DoS protection as it allows to disable slower or less efficient methods. And it doesn't rly make it simpler as it only saves a bit of parsing but requires some ifdefed constant or smth.

@ldpl ldpl force-pushed the compression-handshake branch 2 times, most recently from 98e4090 to 235ed77 Compare Mar 6, 2021
@ldpl
Copy link
Contributor Author

@ldpl ldpl commented Mar 6, 2021

Not that I'm completely happy with it but at least it looks decent now. Anyway I'm out of ideas for how to improve it so I guess it's ready for a review.

@ldpl ldpl marked this pull request as ready for review Mar 6, 2021
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@glx22 glx22 left a comment

And I just noticed I had started a review, but never submitted it.

src/network/network_client.cpp Outdated Show resolved Hide resolved
@ldpl ldpl force-pushed the compression-handshake branch from 235ed77 to 4a4a8c9 Compare Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants