CreeperHost / FTB-Backups-2

7 stars 9 forks source link

Add TAR ZSTD compression support #89

Open layou233 opened 3 weeks ago

layou233 commented 3 weeks ago

This PR adds ZSTD algorithm (standard compression level) support for backup.

Tested on my 1.20.1 modded server. World in size of 416.6MB costs ~11 sec to compress using zip, but takes ~6 sec using ZSTD, which is a 80%~90% improvement in compression time and 20MiB more in archive size as a tradeoff.

Things I would also like to do in this PR (but need agreement):

  1. Set ZSTD as default in configuration.

Some thoughts and questions:

  1. I used aircompressor's pure Java implementation instead of more popular luben's zstd-jni binding. I hate embeding libs of all platforms to jar so I did this choice, and let JIT to optimize the performance. So the first backup could be mush slower, and much faster in later backups. This can be changed anyway if you disagree with me.
  2. I used JTar instead of Apache Common Compress because Apache depends a lot and make the jar much fatter. It's a bad Apache culture.
  3. Is shadowJar's minimization feature useful in this project? I see a lot of unused code that could be tree-shaken especially in this PR.
  4. I saw the deprecated GZip code in the FileUtils.java. Should we remove that?
CLAassistant commented 3 weeks ago

CLA assistant check
All committers have signed the CLA.

covers1624 commented 3 weeks ago

Hi, thanks for your contribution, however, we have a few questions.

What specifically are you trying to resolve with this change? Are you running into performance issues? The entire archiving process is occuring on a sperate thread, basically making the compression time saves irrelivent.

This is a lot of maintainence burden for not really much substantial changed.

layou233 commented 3 weeks ago

@covers1624 Thank you for your reply!

It just introduces a more modern algorithm and uses some existing code base. Existing issues have pointed out that they take too much time to back up some large worlds (such as #81), and the long time spent may make the process uncontrollable and occupy CPU for a long time.

The dependencies used are unlikely to have serious vulnerabilities (one is stable for 2 years) and only need to be updated on a regular schedule (such as Minercraft version updates).

ThePaul-T commented 3 weeks ago

Is there a specific reason for this library over apache commons? Just trying to figure it all out, rather than making assumptions it feels better to understand your reasons for the specific implementation before making any decisions.

layou233 commented 3 weeks ago

Is there a specific reason for this library over apache commons?

@ThePaul-T Apache have zstd and tar packages, but:

  1. They don't have a actual zstd implementation. It's a wrapper for luben's zstd-jni. https://github.com/apache/commons-compress/blob/b52fda40f4175062e8ef49f4b234d27a71ccc9ef/src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdCompressorOutputStream.java#L25

  2. I first wrote it using Apache's tar, but later I found that packaging with shadowJar depends a lot of things, while JTar is relatively lightweight. It's a ~2MB diff for jar size. So I migrated. If there are bugs I'll migrate it back.