8341141: Optimize DirectCodeBuilder #21243

wenshao · 2024-09-29T00:02:38Z

Some DirectCodeBuilder related optimizations to improve startup and running performance:

Merge calls, merge writeU1 and writeU2 into writeU3
Merge calls, merge writeU1 and writeIndex operations
Directly use writeU1 instead of writeBytecode
Rewrite the implementation of load and store

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-8341141: Optimize DirectCodeBuilder (Sub-task - P4)

Reviewers

Chen Liang (@liach - Reviewer)
Claes Redestad (@cl4es - Reviewer) Review applies to cd597a2a

Contributors

Claes Redestad <redestad@openjdk.org>
Chen Liang <liach@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21243

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-29T00:03:29Z

👋 Welcome back swen! 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 · 2024-09-29T00:04:33Z

@wenshao 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:

8341141: Optimize DirectCodeBuilder

Co-authored-by: Claes Redestad <redestad@openjdk.org>
Co-authored-by: Chen Liang <liach@openjdk.org>
Reviewed-by: liach, redestad

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 35 new commits pushed to the master branch:

62acc9c: 8341548: More concise use of classfile API
7312eea: 8341131: Some jdk/jfr/event/compiler tests shouldn't be executed with Xcomp
966eb72: 8341447: Open source closed frame tests # 5
b9db74a: 8341378: Open source few TrayIcon tests - Set8
6546353: 8340203: Link color is hard to distinguish from text color in API documentation
580eb62: 8320500: [vectorapi] RISC-V: Optimize vector math operations with SLEEF
4a12f5b: 8341643: G1: Merged cards counter skewed by merge cards cache
6e48618: 8341644: Compile error in cgroup coding when using toolchain clang
7a1e832: 8336843: Deprecate java.util.zip.ZipError for removal
f62dba3: 8341597: ZipFileInflaterInputStream input buffer size uses uncompressed size
... and 25 more: https://git.openjdk.org/jdk/compare/1c3e56c3e45be3626afec0461d4ae8059b0b577f...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.

openjdk · 2024-09-29T00:04:56Z

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

mlbridge · 2024-09-29T02:17:28Z

Webrevs

src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java

src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java

Reviewed-by: jjg

Reviewed-by: lancea, eirbjo

…racter literals Reviewed-by: mcimadamore

Reviewed-by: azafari, yan, gziemski

Reviewed-by: mr, jwaters, erikj

…mitDirectMemory.java to problem list Reviewed-by: liach, dcubed, alanb

…no test coverage Reviewed-by: lancea

Reviewed-by: prr

…osx-all Reviewed-by: liach, alanb, darcy, dfuchs

…ad of AtomicInteger Reviewed-by: jrose, redestad, shade

…dles.Lookup Reviewed-by: redestad

Reviewed-by: shade, kvn

Reviewed-by: dnguyen, prr

… thread safe Reviewed-by: liach, shade, jvernee

Reviewed-by: asotona

cl4es · 2024-10-07T19:39:46Z

I took this for a spin and results looks promising. Even a tentative win on the Write.jdkTree microbenchmark. The number of changes is a bit staggering so will take a while to thoroughly review in detail. I might be able to get through it all tomorrow.

liach

Everything else looks good.

src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java

liach · 2024-10-07T20:03:29Z

src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java

        return this;
    }

    @Override
    public CodeBuilder aload(int slot) {
-        writeLocalVar(BytecodeHelpers.aload(slot), slot);
+        if (slot >= 0 && slot <= 3) {


Should we use if ((slot & ~3) != 0) for shorter bytecode? #21367

While fun, I wonder if such bit-fiddling optimizations obfuscate the code more than it helps performance. Would be good with some supporting evidence that it 1) helps interpreted performance and 2) that all JITs recognize and optimize this pattern well and without surprises.

liach · 2024-10-07T20:08:42Z

src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java

+        if (slot < 256) {
+            bytecodesBufWriter.writeU1U1(bytecode, slot);
+        } else {
+            bytecodesBufWriter.writeU1U1U2(WIDE, bytecode, slot);
        }


Can we do:

if ((slot & ~0xFF) == 0) bytecodesBufWriter.writeU1U1(bytecode, slot); else if ((slot & ~0xFFFF) == 0) bytecodesBufWriter.writeU1U1U2(WIDE, bytecode, slot); else throw BytecodeHelpers.slotOutOfBounds(slot);

wenshao · 2024-10-08T11:00:56Z

/contributor add redestad

wenshao · 2024-10-08T11:01:19Z

/contributor add liach

openjdk · 2024-10-08T11:01:28Z

@wenshao
Contributor Claes Redestad <redestad@openjdk.org> successfully added.

openjdk · 2024-10-08T11:01:47Z

@wenshao
Contributor Chen Liang <liach@openjdk.org> successfully added.

cl4es

LGTM.

I spotted places outside of the DirectCodeBuilder paths that could benefit from using these new coalescing writers, but write-only cases is a good focus point for now.

cl4es · 2024-10-08T09:34:42Z

src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java

        } else {
-            bytecodesBufWriter.writeU1(value);
+            bytecodesBufWriter.writeU1U1(opcode.bytecode(), value);
        }
    }

    public void writeLoadConstant(Opcode opcode, LoadableConstantEntry value) {
        // Make sure Long and Double have LDC2_W and
        // rewrite to _W if index is > 256


Pre-existing..

Comment should say >= 256.

As we're cloning from a pre-existing pool I assume there's a (perhaps unlikely) possibility we go from a wide to a lower index? In that case the opcode could profitably be "rewritten" to Opcode.LDC in an else clause. (I assume LDC_W with an index in the 0-255 range works fine functionally; it just wastes a byte.)

Indeed, the opcode here is pointless if we have a pool adaption; in that case we should just use ldc(LoadableConstantEntry).

Here in writeLoadConstant, some branches have curly braces, and some don't. The style is inconsistent. Should I change it?

I think we can do it in a later cleanup that fixes writeLoadConstant. This patch is already huge.

wenshao · 2024-10-08T22:25:03Z

LGTM.

I spotted places outside of the DirectCodeBuilder paths that could benefit from using these new coalescing writers, but write-only cases is a good focus point for now.

Are you talking about merging multiple writeU2 in places like AbstractAttributeMapper? I am planning to make a new PR to complete these works, because this PR has too many changes.

cl4es · 2024-10-08T22:45:59Z

Are you talking about merging multiple writeU2 in places like AbstractAttributeMapper? I am planning to make a new PR to complete these works, because this PR has too many changes.

Good. I would have suggested the same.

wenshao · 2024-10-09T10:00:56Z

/integrate

openjdk · 2024-10-09T10:01:22Z

Going to push as commit 047c2d7.
Since your change was applied there have been 41 commits pushed to the master branch:

d636e0d: 8341688: Aarch64: Generate comments in -XX:+PrintInterpreter to link to source code
d3f3c6a: 8330157: C2: Add a stress flag for bailouts
d809bc0: 8341658: RISC-V: Test DateFormatProviderTest.java run timeouted
de90204: 8341588: Remove CollectionUsageThreshold.java from ProblemList-Xcomp for debugging
f276f58: 8341803: ProblemList containers/docker/TestJcmdWithSideCar.java on linux-x64
7eab0a5: 8337066: Repeated call of StringBuffer.reverse with double byte string returns wrong result
62acc9c: 8341548: More concise use of classfile API
7312eea: 8341131: Some jdk/jfr/event/compiler tests shouldn't be executed with Xcomp
966eb72: 8341447: Open source closed frame tests # 5
b9db74a: 8341378: Open source few TrayIcon tests - Set8
... and 31 more: https://git.openjdk.org/jdk/compare/1c3e56c3e45be3626afec0461d4ae8059b0b577f...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-09T10:01:30Z

@wenshao Pushed as commit 047c2d7.

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

optimize DirectCodeBuilder

effb385

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 29, 2024

remove DirectCodeBuilder::writeLocalVar

2c3d4f1

wenshao changed the title ~~Optimize DirectCodeBuilder~~ 8341141: Optimize DirectCodeBuilder Sep 29, 2024

openjdk bot added the rfr Pull request is ready for review label Sep 29, 2024

wenshao added 3 commits September 29, 2024 17:40

use array instead of ArrayList

3cb1657

bug fix

9e2e4db

use array instead of ArrayList

dab40af

cl4es reviewed Sep 29, 2024

View reviewed changes

src/java.base/share/classes/jdk/internal/classfile/impl/BufWriterImpl.java Outdated Show resolved Hide resolved

liach reviewed Sep 29, 2024

View reviewed changes

wenshao and others added 16 commits September 30, 2024 08:57

suggestion from @cl4es and @liach

2ca81a3

8339541: CSS rule is not specific enough

98d4cce

Reviewed-by: jjg

8340885: Desugar ZipCoder.Comparison

ca6493e

Reviewed-by: lancea, eirbjo

8340568: Incorrect escaping of single quotes when pretty-printing cha…

8395084

…racter literals Reviewed-by: mcimadamore

8338583: NMT: Malloc overhead is calculated incorrectly

ccb18aa

Reviewed-by: azafari, yan, gziemski

8340815: Add SECURITY.md file

7d4b9d5

Reviewed-by: mr, jwaters, erikj

8340946: Add vmTestbase/gc/memory/Nio/Nio.java and java/nio/Buffer/Li…

ae06209

…mitDirectMemory.java to problem list Reviewed-by: liach, dcubed, alanb

8340684: Reading from an input stream backed by a closed ZipFile has …

e2baf83

…no test coverage Reviewed-by: lancea

8340228: Open source couple more miscellaneous AWT tests

08b0b5c

Reviewed-by: prr

8340956: ProblemList 4 java/nio/channels/DatagramChannel tests on mac…

5bd48ed

…osx-all Reviewed-by: liach, alanb, darcy, dfuchs

8340838: Clean up MutableCallSite to use explicit release fence inste…

7031fea

…ad of AtomicInteger Reviewed-by: jrose, redestad, shade

8340831: Simplify simple validation for class definition in MethodHan…

f9fb2c6

…dles.Lookup Reviewed-by: redestad

8340864: Remove unused lines related to vmClasses

d83504d

Reviewed-by: shade, kvn

8339271: giflib attribution correction

b7ae561

Reviewed-by: dnguyen, prr

8340812: LambdaForm customization via MethodHandle::updateForm is not…

dc2e4a0

… thread safe Reviewed-by: liach, shade, jvernee

8339260: Move rarely used constants out of ClassFile

140dc49

Reviewed-by: asotona

wenshao requested a review from liach October 5, 2024 15:35

wenshao and others added 2 commits October 5, 2024 23:41

Merge branch 'master' into optim_direct_code_builder_202409

d4752d5

fix merge error

0bcedf8

cl4es mentioned this pull request Oct 7, 2024

Fold TAG_UTF8 write into writeUTF wenshao/jdk#15

Merged

liach reviewed Oct 7, 2024

View reviewed changes

wenshao and others added 5 commits October 8, 2024 06:50

suggestion from @liach

c6e4114

Fold TAG_UTF8 write into writeUTF

26ec184

Rename writeUtfEntry

0cfbae7

A few missed places, U1U2U2 seem common enough too to be included

ad9d7a0

Fold opcode.bytecode() in writeLoadConstant

cd597a2

liach approved these changes Oct 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 8, 2024

cl4es approved these changes Oct 8, 2024

View reviewed changes

fix comment

894f794

liach approved these changes Oct 8, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 9, 2024

openjdk bot closed this Oct 9, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 9, 2024

wenshao mentioned this pull request Oct 10, 2024

8341906: Optimize ClassFile writing BufBuffer #21437

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8341141: Optimize DirectCodeBuilder #21243

8341141: Optimize DirectCodeBuilder #21243

wenshao commented Sep 29, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 29, 2024

openjdk bot commented Sep 29, 2024 •

edited

Loading

openjdk bot commented Sep 29, 2024

mlbridge bot commented Sep 29, 2024 •

edited

Loading

cl4es commented Oct 7, 2024

liach left a comment

liach Oct 7, 2024 •

edited

Loading

cl4es Oct 7, 2024

liach Oct 7, 2024

wenshao commented Oct 8, 2024

wenshao commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

cl4es left a comment

cl4es Oct 8, 2024 •

edited

Loading

liach Oct 8, 2024

wenshao Oct 8, 2024

liach Oct 8, 2024

wenshao commented Oct 8, 2024

cl4es commented Oct 8, 2024

wenshao commented Oct 9, 2024

openjdk bot commented Oct 9, 2024

openjdk bot commented Oct 9, 2024

8341141: Optimize DirectCodeBuilder #21243

8341141: Optimize DirectCodeBuilder #21243

Conversation

wenshao commented Sep 29, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Sep 29, 2024

openjdk bot commented Sep 29, 2024 • edited Loading

openjdk bot commented Sep 29, 2024

mlbridge bot commented Sep 29, 2024 • edited Loading

Webrevs

cl4es commented Oct 7, 2024

liach left a comment

Choose a reason for hiding this comment

liach Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

cl4es Oct 7, 2024

Choose a reason for hiding this comment

liach Oct 7, 2024

Choose a reason for hiding this comment

wenshao commented Oct 8, 2024

wenshao commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

cl4es left a comment

Choose a reason for hiding this comment

cl4es Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

liach Oct 8, 2024

Choose a reason for hiding this comment

wenshao Oct 8, 2024

Choose a reason for hiding this comment

liach Oct 8, 2024

Choose a reason for hiding this comment

wenshao commented Oct 8, 2024

cl4es commented Oct 8, 2024

wenshao commented Oct 9, 2024

openjdk bot commented Oct 9, 2024

openjdk bot commented Oct 9, 2024

wenshao commented Sep 29, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 29, 2024 •

edited

Loading

mlbridge bot commented Sep 29, 2024 •

edited

Loading

liach Oct 7, 2024 •

edited

Loading

cl4es Oct 8, 2024 •

edited

Loading