8316582: Minor startup regression in 22-b15 due JDK-8310929

[cl4es]

This patch reverts the use of ByteArrayLittleEndian in StringLatin1.

This use is the cause of a small (~1.5ms) startup regression in 22-b15. While a manageable startup regression in and of itself, the use of VarHandles in core utility classes brings an increased risk of bootstrap circularity issues, for example disqualifying the use of things like Integers.toString in some places.

Reverting this partially rolls back the performance improvement gained by JDK-8310929. It seems reasonable that the compiler can be enhanced to gain that loss back.

---

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-8316582: Minor startup regression in 22-b15 due JDK-8310929 (Bug - P4)

Reviewers

• Chen Liang (@liach - Author)
• Roger Riggs (@RogerRiggs - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15836

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

Using diff file

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

[bridgekeeper]

👋 Welcome back redestad! 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]

@cl4es 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.

[cl4es]

This PR vs a 22-b15 baseline:

Name                   Cnt  Base   Error   Test   Error  Unit   Diff%
Integers.toStringBig    15 5,318 ± 0,043  6,628 ± 0,127 us/op  -24,6% (p = 0,000*)
Integers.toStringSmall  15 3,202 ± 0,018  3,562 ± 0,027 us/op  -11,2% (p = 0,000*)
Integers.toStringTiny   15 2,286 ± 0,017  2,352 ± 0,024 us/op   -2,9% (p = 0,000*)
  * = significant

This PR vs a 22-b14 baseline:

Name                   Cnt   Base   Error   Test   Error  Unit   Diff%
Integers.toStringBig    15 12,313 ± 0,143  6,628 ± 0,127 us/op   46,2% (p = 0,000*)
Integers.toStringSmall  15  4,816 ± 0,074  3,562 ± 0,027 us/op   26,0% (p = 0,000*)
Integers.toStringTiny   15  2,611 ± 0,022  2,352 ± 0,024 us/op    9,9% (p = 0,000*)
  * = significant

There's still a substantial win compared to 22-b14, stemming from the use of a packed lookup table rather than two disjoint tables for tens and single digit numbers.

[liach]

Can #14636 be a solution to avoid early VH initialization?

Also curious how you created such a "Base vs Test" metrics table, could you teach me how?

[cl4es]

Can #14636 be a solution to avoid early VH initialization?

I think #14636 would more or less solve the startup regression, yes, but the jury is out on whether we want to accept that. There's value in taking steps to make VH indistinguishable from Unsafe w.r.t. start-up and other performance characteristics. The issue with using ByteArray* in Integer/StringLatin1 is also more one of bootstrap dependencies.

We also really need to analyze why the JIT doesn't generate as good code for pair-wise byte[] stores as for ByteArray.setShort and then try to fix this in the JIT. This would be much preferable to having to pull out a power tool like ByteArrayLittleEndian for something so trivial. It might be a good exercise for anyone who want to dive into JITs to get these to perform equally.

Also curious how you created such a "Base vs Test" metrics table, could you teach me how?

It's a tool I've written that parses JMH json output and allows comparisons. It's currently part of a larger benchmark running toolkit that we probably can't open source, but I could probably extract the simple parser + printer and put it under the OpenJDK somewhere if there's interest.

[cl4es]

Startup numbers improve with the above patch to levels on par with 22-b14:

Name                Cnt          Base         Error           Test         Error         Unit   Diff%
Perfstartup-Noop-G1  20        30,000 ±       0,000         28,500 ±       3,181        ms/op    5,0% (p = 0,083 )
  :.cycles           20  88166516,750 ± 2119868,114   84226439,550 ± 1792195,203       cycles   -4,5% (p = 0,000*)
  :.instructions     20 204321816,400 ±  248867,819  195313416,200 ±  196361,902 instructions   -4,4% (p = 0,000*)
  :.taskclock        20        12,000 ±       4,543         10,000 ±       0,000           ms  -16,7% (p = 0,104 )
  * = significant

(This is simply a Noop/Hello World program in a loop, with stats collected by /usr/bin/time -l, run on a MacBook M1)

[liach]

Looks good. I also wonder if storing digit pairs in platform endianness is better; currently only Unsafe support writing multibytes with platform endianness.

[cl4es]

FWIW when initializing DIGITS directly (DIGITS = new byte[] { ...) the DecimalDigits class is 2610 bytes, with the for loop in a static block it drops down to 2112 bytes. Array constants like this generate sad and bloated bytecode:

         0: bipush        100
         2: newarray       short
         4: dup
         5: iconst_0
         6: sipush        12336
         9: sastore
         ...
        40: dup
        41: bipush        6
        43: sipush        13872
        46: sastore
        ...
       691: dup
       692: bipush        99
       694: sipush        14649
       697: sastore
       698: putstatic     #13                 // Field DIGITS:[S

[RogerRiggs]

Thanks for tracking down the regression.

[cl4es]

/integrate

[openjdk]

@cl4es Your integration request cannot be fulfilled at this time, as the status check jcheck-openjdk/jdk-15836 did not complete successfully

[openjdk]

@cl4es This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8316582: Minor startup regression in 22-b15 due JDK-8310929

Reviewed-by: liach, rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 23 new commits pushed to the master branch:

• 23ed890: 6415065: Submenu is shown on wrong screen in multiple monitor environment
• ca47f5f: 8316105: C2: Back to back Parse Predicates from different loops but with same deopt reason are wrongly grouped together
• 1749ba2: 8311084: Add typeSymbol() API for applicable constant pool entries
• 9f5d2b9: 8316285: Opensource JButton manual tests
• a35e96a: 8313612: Use JUnit in lib-test/jdk tests
• bee7524: 8315786: [AIX] Build Disk Local Detection Issue with GNU-utils df on AIX
• ceff47b: 8315082: [REDO] Generational ZGC: Tests crash with assert(index == 0 || is_power_of_2(index))
• df4a25b: 8308762: Metaspace leak with Instrumentation.retransform
• 8412479: 8316229: Enhance class initialization logging
• c04c9ea: 8316627: JViewport Test headless failure
• ... and 13 more: https://git.openjdk.org/jdk/compare/ec74194cb75afcaab2f77e8728391bb9104ccc73...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[cl4es]

/integrate

[openjdk]

Going to push as commit 913e43f.
Since your change was applied there have been 23 commits pushed to the master branch:

• 23ed890: 6415065: Submenu is shown on wrong screen in multiple monitor environment
• ca47f5f: 8316105: C2: Back to back Parse Predicates from different loops but with same deopt reason are wrongly grouped together
• 1749ba2: 8311084: Add typeSymbol() API for applicable constant pool entries
• 9f5d2b9: 8316285: Opensource JButton manual tests
• a35e96a: 8313612: Use JUnit in lib-test/jdk tests
• bee7524: 8315786: [AIX] Build Disk Local Detection Issue with GNU-utils df on AIX
• ceff47b: 8315082: [REDO] Generational ZGC: Tests crash with assert(index == 0 || is_power_of_2(index))
• df4a25b: 8308762: Metaspace leak with Instrumentation.retransform
• 8412479: 8316229: Enhance class initialization logging
• c04c9ea: 8316627: JViewport Test headless failure
• ... and 13 more: https://git.openjdk.org/jdk/compare/ec74194cb75afcaab2f77e8728391bb9104ccc73...master

Your commit was automatically rebased without conflicts.

[openjdk]

@cl4es Pushed as commit 913e43f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mlbridge]

Webrevs

• 00: Full (8c2a1e92)

[wenshao]

digitPair and Unsafe.putShort are a good combination. Can Unsafe be used here?

   private static void writeDigitPair(byte[] buf, int charPos, int value) {
        UNSAFE.putShortUnaligned(
                buf,
                Unsafe.ARRAY_BYTE_BASE_OFFSET + charPos,
                digitPair(value),
                false);
    }

[cl4es]

I'd like us to take a step back and instead of reaching for and sprinkling Unsafe, ByteArrayLittleEndian and VarHandles all over the place to instead consider differences in performance on trivial code like this as compiler bugs. We need to investigate if there's anything we can do to have the JIT generate better code here for idiomatic java, and only once we thoroughly understand if and why that's not possible to reach for other options.