8310837: Use ByteArrayLittleEndian in java.util.zip

[Glavo]

Using ByteArrayLittleEndian is simpler and faster.

make test TEST="micro:java.util.zip.ZipFileOpen":

  Benchmark                     (size)  Mode  Cnt      Score      Error  Units
- ZipFileOpen.openCloseZipFile     512  avgt   15  39052.832 ±  107.496  ns/op
+ ZipFileOpen.openCloseZipFile     512  avgt   15  36275.539 ±  663.193  ns/op
- ZipFileOpen.openCloseZipFile    1024  avgt   15  77106.494 ± 4159.300  ns/op
+ ZipFileOpen.openCloseZipFile    1024  avgt   15  71955.013 ± 2296.050  ns/op

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8310837: Use ByteArrayLittleEndian in java.util.zip (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14632/head:pull/14632
$ git checkout pull/14632

Update a local copy of the PR:
$ git checkout pull/14632
$ git pull https://git.openjdk.org/jdk.git pull/14632/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14632

View PR using the GUI difftool:
$ git pr show -t 14632

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14632.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back Glavo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@Glavo The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[liach]

Looks great. Created https://bugs.openjdk.org/browse/JDK-8310837 for your patch.

[mlbridge]

Webrevs

• 07: Full - Incremental (95c29249)
• 06: Full - Incremental (a9b6b78e)
• 05: Full - Incremental (72175ea1)
• 04: Full - Incremental (efdc185b)
• 03: Full - Incremental (49520bbf)
• 02: Full - Incremental (c14b9620)
• 01: Full - Incremental (6ba44e49)
• 00: Full (bd67e7a3)

[AlanBateman]

@LanceAndersen This one is going to require checking that startup isn't impacted.

[jaikiran]

Hello Glavo, I was going to recommend adding a test method to existing jtreg tests to test these new methods. But it looks like there's no jtreg test for this ByteArrayLittleEndian class. Would you mind creating a new test class to (at least) test these methods?

[Glavo]

Hello Glavo, I was going to recommend adding a test method to existing jtreg tests to test these new methods. But it looks like there's no jtreg test for this ByteArrayLittleEndian class. Would you mind creating a new test class to (at least) test these methods?

I think I could modify test/jdk/jdk/internal/util/ByteArray/ReadWriteValues.java to also test ByteArrayLittleEndian.

[Glavo]

I added a test for ByteArrayLittleEndian in #14636

[jaikiran]

On an existing Java runtime, I had a quick look at the classes loaded at application startup to see if MethodHandles already gets loaded before ZipUtils (and related jar/zip classes). From what I see in the output, MethodHandles indeed is already loaded before ZipUtils. So I think this change doesn't bring in any new/unexpected usage of MethodHandles that could potentially impact startup performance.

P.S: I just noticed Alan has a similar comment about startup performance, so yes, having some numbers for a startup benchmark would be useful.

[Glavo]

@AlanBateman @jaikiran Sorry, I thought about its possible impact on startup time, but I don't know which tests can be used to test JVM startup time. Can you tell me some relevant tests?

In fact, I now have a branch(#14636) that rewrites ByteArray and ByteArrayLittleEndian to avoid using VarHandle because I hope to be able to use them more inside the JDK later.

I tested its performance with the -Xint option on and it is indeed faster than VarHandle in interpreter mode. But I think I'm missing some benchmark results of the impact on startup time to justify its necessity.

[jaikiran]

Hello Glavo,

@AlanBateman @jaikiran Sorry, I thought about its possible impact on startup time, but I don't know which tests can be used to test JVM startup time. Can you tell me some relevant tests?

In fact, I now have a branch(#14636) that rewrites ByteArray and ByteArrayLittleEndian to avoid using VarHandle because I hope to be able to use them more inside the JDK later.

I think this PR should then wait for a bit to see what comes out of #14636. That will then help us understand what kind of additional testing might have to be considered for this current PR.

[LanceAndersen]

@LanceAndersen This one is going to require checking that startup isn't impacted.

@LanceAndersen This one is going to require checking that startup isn't impacted.

ZipFS has a slightly different macro for SH:

static final int SH(byte[] b, int n) {
      return Byte.toUnsignedInt(b[n]) | (Byte.toUnsignedInt(b[n + 1]) << 8);
  }

I think as we compare performance, we also should strive for consistency between Zip and ZipFS implementation as well

[Glavo]

I think as we compare performance, we also should strive for consistency between Zip and ZipFS implementation as well

Can we move ZipUtils, ZipCoder, and ZipConstants to other package (such as jdk.internal.zip)? Then we can share them between java.util.zip and zipfs.

If there's no reason to stop us from doing this, I can open a new PR to do this after this PR is merged.

[AlanBateman]

If there's no reason to stop us from doing this, I can open a new PR to do this after this PR is merged.

We shouldn't rush into that as it it creates coupling between java.base and jdk.zipfs that is problematic for maintenance, integrity and security. Need to be careful with every qualified exports that is added.

[Glavo]

We shouldn't rush into that as it it creates coupling between java.base and jdk.zipfs that is problematic for maintenance, integrity and security. Need to be careful with every qualified exports that is added.

Oh well. I'm just confused as to why these few classes are being duplicated, is it for historical reasons?

Another question that confuses me is why ZipUtils needs to use Unsafe to get the hb and offset fields of ByteBuffer. Is it because of performance?

[bridgekeeper]

@Glavo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@Glavo This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[Glavo]

/open

[openjdk]

@Glavo This pull request is now open

[bridgekeeper]

@Glavo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[liach]

@LanceAndersen This one is going to require checking that startup isn't impacted.

Now that ByteArrayLittleEndian is used in Integer toString, don't think this patch will affect the startup class loading sequence.

[bridgekeeper]

@Glavo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@Glavo This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[Glavo]

/open

[openjdk]

@Glavo This pull request is now open

[bridgekeeper]

@Glavo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[jaikiran]

Hello Glavo, I see that you are interested in pursuing this change further. Would you mind getting the latest micro benchmark numbers which this proposed change? I see that your PR description has a run from some time back, getting a latest one would be useful.

Additionally, I see that #14636 where you had proposed a test case for the ByteArrayLittleEndian class (in addition to other things) got closed without being integrated. Would you mind adding a new test case for that class as part of this current PR since you have a few more new methods being added to that class?

[Glavo]

Hello Glavo, I see that you are interested in pursuing this change further. Would you mind getting the latest micro benchmark numbers which this proposed change? I see that your PR description has a run from some time back, getting a latest one would be useful.

Additionally, I see that #14636 where you had proposed a test case for the ByteArrayLittleEndian class (in addition to other things) got closed without being integrated. Would you mind adding a new test case for that class as part of this current PR since you have a few more new methods being added to that class?

I've moved those changes into this PR and am running tests. I'll push these changes once the tests are finished running.

[Glavo]

I ran the tier1 tests with no failures.

[Glavo]

@jaikiran Can you review this PR again?

[bridgekeeper]

@Glavo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[jaikiran]

Hello Glavo, I'll need some more time on this to review this. In the meantime, could you update the micro benchmark latest numbers with this latest state?

[minborg]

Nice with better testing for the utility classes.

[LanceAndersen]

As mentioned earlier, it would be beneficial to measure the potential benefit of this proposed change with ZipFS which has similar albeit slightly different approach to the macros and include as part of this PR of a follow-on PR

[Glavo]

Hello Glavo, I'll need some more time on this to review this. In the meantime, could you update the micro benchmark latest numbers with this latest state?

Latest results:

  Benchmark                       (size)  Mode  Cnt      Score        Error  Units
- ZipFileOpen.openCloseZipFile       512  avgt   15    55411.637 ± 2010.322  ns/op
+ ZipFileOpen.openCloseZipFile       512  avgt   15    53247.244 ±  602.826  ns/op
- ZipFileOpen.openCloseZipFile      1024  avgt   15    99983.208 ±  374.463  ns/op
+ ZipFileOpen.openCloseZipFile      1024  avgt   15    96463.939 ±  273.324  ns/op
- ZipFileOpen.openCloseZipFilex2     512  avgt   15    57308.739 ± 1627.708  ns/op
+ ZipFileOpen.openCloseZipFilex2     512  avgt   15    54735.296 ±  241.473  ns/op
- ZipFileOpen.openCloseZipFilex2    1024  avgt   15   102068.887 ±  318.778  ns/op
+ ZipFileOpen.openCloseZipFilex2    1024  avgt   15    98095.240 ±  319.524  ns/op

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[bridgekeeper]

@Glavo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@Glavo This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[Glavo]

/open

[openjdk]

@Glavo This pull request is now open

[liach]

@cl4es Can you take a look at this performance improvement patch?

[Glavo]

I've been busy working on other projects lately, so I've put these PRs on hold. If anyone cares to take a look at them I'll be right back.

[bridgekeeper]

@Glavo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@Glavo This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.