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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

ldpl
Copy link
Contributor

@ldpl ldpl commented Mar 4, 2021 •
edited

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')

JGRennison
JGRennison reviewed Mar 4, 2021
View reviewed changes
src/table/settings.ini Outdated Show resolved Hide resolved
@JGRennison
Copy link
Contributor

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 commented Mar 4, 2021 •
edited

@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 commented Mar 4, 2021 •
edited

@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 March 6, 2021 19:23
@ldpl
Copy link
Contributor Author

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 March 6, 2021 19:26
@TrueBrain TrueBrain mentioned this pull request Mar 11, 2021
Closed
ldpl
ldpl commented Apr 2, 2021
View reviewed changes
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
glx22
glx22 reviewed Apr 2, 2021
View reviewed changes
Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/network/network_client.cpp Outdated Show resolved Hide resolved
@2TallTyler 2TallTyler added the work: needs rebase This Pull Request needs a rebase label Nov 9, 2022
@TrueBrain
Copy link
Member

As mentioned in #8773, this PR only has merit if there is more than one compression being offered that are useful. As there currently aren't, let's close this up for now, and reopen when we ever need it again.

@TrueBrain TrueBrain closed this Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glx22 glx22 glx22 left review comments

@JGRennison JGRennison JGRennison left review comments

Assignees
No one assigned
Labels
work: needs rebase This Pull Request needs a rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ldpl @JGRennison @James103 @TrueBrain @glx22 @2TallTyler