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

Add Zstd/LZ4 support for region files (1.16.4 edition) #5029

Closed
wants to merge 3 commits into from

Conversation

egg82
Copy link
Contributor

@egg82 egg82 commented Jan 8, 2021

Zstd using zstd-jni: https://github.com/luben/zstd-jni
LZ4 using lz4-java: https://github.com/lz4/lz4-java

Added new command-line option, --forceWrite, which will forcibly write the chunk/flush to disk regardless of whether or not the chunk has actually changed in a --forceUpgrade.
New compression only happens when a chunk is saved, so running a --forceUpgrade AND --forceWrite may be desirable.

New (hidden) config option for the compression method to use.
There's a new option in PaperConfig (compression level) for Zstd which defaults to 6.
There's a new option in PaperConfig (compression level) for Zlib which defaults to 5.
Added a getter to the API to retrieve the current chunk compression method being used.

Since the patch hooks Mojang's versioning system for new compression methods, it
should be internally consistent, easily-updatable, and reversible with a --forceUpgrade AND a --forceWrite.

This will likely conflict with plugins and programs expecting region files to be compressed using the current standard.

Supercedes #4715

Copy link
Member

@Spottedleaf Spottedleaf left a comment

Choose a reason for hiding this comment

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

Due to GC concerns with how the library is written, the direct bytebuffer options must be used for ZSTD.

@egg82
Copy link
Contributor Author

egg82 commented Jan 8, 2021

@Spottedleaf for posterity, is there a particular issue on the zstd-jni Github that we can link for that?

@MeFisto94
Copy link
Contributor

In regards of the custom trained dictionary:
How specific is this?
Like will the improvements come with downsides for say citybuild servers having a different block distribution than vanilla?
Or are most gains based on the general region format and thus "content independent"?

@Titaniumtown
Copy link
Contributor

I use this patch on my fork and I've had no issues with it whatsoever, it just took a few hours to force upgrade my world. More testing is probably needed though.

@electronicboy
Copy link
Member

The patch, in general, is fine, afaik the only real remaining issues are tryna deal with the side-effects of ID 10 T

@Titaniumtown
Copy link
Contributor

Titaniumtown commented Feb 4, 2021

What's ID 10 T?

Edit: It's come to my attention I just got played.

@electronicboy
Copy link
Member

Unless there are any objections, I think I'm generally happy with this, at least, as much as I'm gonna be

@Owen1212055
Copy link
Member

Closing PR, as it seems that this has gone inactive.

If you're still interested in continuing this PR, feel free to rebase and leave a comment and we can reopen it for you. 😄
But since it seems there is a lot of interest in this, if anyone else instead wants to pick up on this additional PRs are welcome!

@NoahvdAa
Copy link
Member

NoahvdAa commented Oct 22, 2022

iirc the merging was held of for two reasons (I believe we discussed this in a VC at some point, if my memory serves me right):

  • 1D10T errors; This format is not compatible with vanilla for obvious reasons. Users may not realize this though, and would potentially convert their saves without a proper backup.
  • Mojang was (at the time, maybe still is?) thinking about adding different compression formats, and this would possibly clash with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants