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

Use buffered IO streams in renamer #4

Merged
merged 11 commits into from
Mar 26, 2024

Conversation

kennytv
Copy link
Contributor

@kennytv kennytv commented Mar 23, 2024

This results in a reduction of 1/2 to 2/3 of the total time spent in the run method for large files (e.g. 12 seconds -> 4 seconds for us when transforming the Minecraft server), and still a sizable improvement for smaller files. If required for any reason, I can put this change behind some useBufferedIO boolean that defaults to false

@kennytv kennytv changed the title Use buffered IO streams in renamer Use buffered IO streams in renamer, update Java target to 17 Mar 24, 2024
@kennytv
Copy link
Contributor Author

kennytv commented Mar 24, 2024

hope you don't mind the extra language changes being in this pr, otherwise that one specific commit can be reverted

@shartte shartte self-requested a review March 26, 2024 13:36
@shartte
Copy link
Contributor

shartte commented Mar 26, 2024

Tried to test this vs. the old version. I have no experience with CLI benchmarking so I used this tool: https://github.com/sharkdp/hyperfine

This is with a very fast NVMe disk, remapping the latest Minecraft snapshot in userdev:

New:
image

Old (1.0.14):
image

So a ~1.5 second runtime improvement in this test.

@shartte shartte requested a review from Matyrobbrt March 26, 2024 15:00
@shartte
Copy link
Contributor

shartte commented Mar 26, 2024

@Matyrobbrt Can you give a quick sanity check? Moving this to J17 isn't an issue, right?

@Matyrobbrt
Copy link
Member

It is an issue, the installer runs ART.

@kennytv
Copy link
Contributor Author

kennytv commented Mar 26, 2024

Benchmarks with gradle are a slightly more annoying, but I used https://github.com/melix/jmh-gradle-plugin. Everything but the output stream change was only really in the tens or hundreds of milliseconds. Some of the individual parts I also simply used good old system.nanoseconds and logging in our real-world use of re-obfuscating the server and de-obfuscating plugins, which showed similar results to more fine-grained benchmarks (like file reading) or that of the full round-trip as below

Though here's a simple (but not exactly ideal) example benchmark of the whole roundtrip checking only the BufferedOutputStream change, not passing any mappings: https://paste.gg/p/anonymous/bee85de132944515afcef9fc8f4c6c34

-- Both of these are based off the current PR status *except* RenamerImpl has been fully reverted + Util readded
-- No change to RenamerImpl
Benchmark   Mode  Cnt     Score     Error  Units
Bench.test  avgt    9  7857,517 ± 589,750  ms/op

-- Use of BufferedOutputStream when writing
Benchmark   Mode  Cnt     Score     Error  Units
Bench.test  avgt    9  1717,324 ± 181,302  ms/op

High-ish error rates because I didn't want the benchmarks to run for ages, but it already shows how much of a difference it makes. Ran on Windows with a semi-recent ssd to represent those 10% of our users on the slow file system:tm:

@shartte
Copy link
Contributor

shartte commented Mar 26, 2024

@kennytv I ported it back to J8. The improvements should still be the same. Can you check if my changes are okay with you? At that point I think we can merge.

@kennytv
Copy link
Contributor Author

kennytv commented Mar 26, 2024

Fixed a small typo and made one last change to close zip entry inputstreams early. That's not technically required as open streams will be stored and closed once ZipFile is closed, but this'll free them up early + allows deflaters to be reused (didn't test the real-world impact yet, but jmh benchmarks showed the average time of reading a Minecraft jar change from 600 to 300ms on Windows).
Otherwise should be good 👍

@sciwhiz12 sciwhiz12 added the enhancement New feature or request label Mar 26, 2024
@sciwhiz12 sciwhiz12 changed the title Use buffered IO streams in renamer, update Java target to 17 Use buffered IO streams in renamer Mar 26, 2024
@shartte shartte merged commit 176a151 into neoforged:main Mar 26, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants