8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms #23766

dbriemann · 2025-02-25T08:43:04Z

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-8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms (Enhancement - P4)

Reviewers

Leonid Mesnik (@lmesnik - Reviewer)
Martin Doerr (@TheRealMDoerr - Reviewer)
Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23766

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-25T08:43:47Z

Hi @dbriemann, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user dbriemann" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2025-02-25T08:44:26Z

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

8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms

Reviewed-by: lmesnik, mdoerr, shade

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

1fe4526: 8350194: Last 2 parameters of ReturnNode::ReturnNode are swapped in the declaration
1d147cc: 8351484: Race condition in max stats in MonitorList::add
4412c07: 8351639: Improve debuggability of test/langtools/jdk/jshell/JdiHangingListenExecutionControlTest.java test
1dd9cf1: 8349099: java/awt/Headless/HeadlessMalfunctionTest.java fails on CI with Compilation error
64464ea: 8351673: Clean up a case of if (LockingMode == LM_LIGHTWEIGHT) in a legacy-only locking mode function
9a49418: 8345940: Migrate security-related resources from Java classes to properties files
e71f327: 8351045: ClassValue::remove cannot ensure computation observes up-to-date state
cef3693: 8351656: Problemlist gc/TestAllocHumongousFragment#generational
da2b4f0: 8351606: Use build_platform for graphviz dependency
030c85d: 8350106: [PPC] Avoid ticks_unknown_not_Java AsyncGetCallTrace() if JavaFrameAnchor::_last_Java_pc not set
... and 30 more: https://git.openjdk.org/jdk/compare/088726238664985ebf2bc60deca96f22245e9ce3...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@lmesnik, @TheRealMDoerr, @shipilev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-02-25T08:44:55Z

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

hotspot

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.

dbriemann · 2025-02-25T11:23:46Z

/covered

bridgekeeper · 2025-02-25T11:24:21Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

myankelev · 2025-02-25T13:57:53Z

test/hotspot/jtreg/runtime/interpreter/CountBytecodesTest.java

+            ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder("-Xint", "-XX:+CountBytecodes", "CountBytecodesTest", "test");
+            OutputAnalyzer output = new OutputAnalyzer(pb.start());


Do you think it would be easier to use ProcessTools.executeTestJava here?

That looks better. I fixed it.

TheRealMDoerr · 2025-02-25T14:03:22Z

@RealFYang, @offamitkumar: You may want to test and review this PR on your platforms.

RealFYang · 2025-02-26T02:51:25Z

@RealFYang, @offamitkumar: You may want to test and review this PR on your platforms.

Hi, Thanks for the ping! RISC-V part of the change looks fine.
And runtime/interpreter/CountBytecodesTest.java test good with fastdebug build on my platform.

offamitkumar · 2025-02-26T03:00:18Z

I don't see any failure on s390x as well.

lmesnik · 2025-03-05T19:39:23Z

test/hotspot/jtreg/runtime/interpreter/CountBytecodesTest.java

+ * @summary Test the output for CountBytecodes and validate that the counter
+ *          does not overflow for more than 2^32 bytecodes counted.
+ * @library /test/lib
+ * @run main/othervm/timeout=300 CountBytecodesTest


The long tests should be excluded from tier1. Please update TEST.groups.

I excluded the test from the tier1_runtime tests. To my understanding it should now run in tier4. Could you please verify? Thanks.

Yes, this should work. In current definition, tier4 is "catch-all" group that handles all the tests that are not explicitly in tier{1,2,3}.

mlbridge · 2025-03-05T21:52:13Z

Webrevs

01: Full - Incremental (31a52156)
00: Full (45699ec5)

TheRealMDoerr

LGTM.

shipilev

This looks fine to me, with a few nits, thanks.

x86_32 parts would go away as we cleanup after x86_32 removal, but they can stay here for completeness and backportability.

shipilev · 2025-03-11T15:39:17Z

src/hotspot/share/interpreter/bytecodeTracer.cpp

      st->print("[%zu] ", Thread::current()->osthread()->thread_id_for_printing());
      if (Verbose) {
-        st->print("%8d  %4d  " INTPTR_FORMAT " " INTPTR_FORMAT " %s",
+        st->print("%8zu  %4d  " INTPTR_FORMAT " " INTPTR_FORMAT " %s",


Sounds like there are more than 8 digits now?

I thought about this, too, but I don't think it's a problem because the width is specified like this: "Minimum number of characters to be printed. If the value to be printed is shorter than this number, the result is padded with blank spaces. The value is not truncated even if the result is larger." [https://cplusplus.com/reference/cstdio/printf/].
Do we want a larger fixed number of digits?

Yeah, it is not about the correctness. It is more about readability: if we expect more than 8 digits, then the "table" we are printing here would be a bit ragged. UINT64_MAX is about 20 digits. In practice we would probably never do this for longer than 1 hour, and with (ballparking) 100M/sec bytecodes, this gives us a practical upper limit of 12 digits or so? My math might be off a digit or two.

Actually, nevermind. I don't think this is useful to adjust. The bytecode counter is global, so it is not a per-bytecode printout like I initially thought.

lmesnik

Test changes looks good for me.

dbriemann · 2025-03-12T10:12:28Z

Thanks for the reviews!

One test timed out ( gc/TestAllocHumongousFragment#generational ) but it is unrelated to this change.

/integrate

openjdk · 2025-03-12T10:13:11Z

@dbriemann
Your change (at version 31a5215) is now ready to be sponsored by a Committer.

TheRealMDoerr · 2025-03-12T10:21:56Z

/sponsor

openjdk · 2025-03-12T10:22:48Z

Going to push as commit 4be502e.
Since your change was applied there have been 40 commits pushed to the master branch:

1fe4526: 8350194: Last 2 parameters of ReturnNode::ReturnNode are swapped in the declaration
1d147cc: 8351484: Race condition in max stats in MonitorList::add
4412c07: 8351639: Improve debuggability of test/langtools/jdk/jshell/JdiHangingListenExecutionControlTest.java test
1dd9cf1: 8349099: java/awt/Headless/HeadlessMalfunctionTest.java fails on CI with Compilation error
64464ea: 8351673: Clean up a case of if (LockingMode == LM_LIGHTWEIGHT) in a legacy-only locking mode function
9a49418: 8345940: Migrate security-related resources from Java classes to properties files
e71f327: 8351045: ClassValue::remove cannot ensure computation observes up-to-date state
cef3693: 8351656: Problemlist gc/TestAllocHumongousFragment#generational
da2b4f0: 8351606: Use build_platform for graphviz dependency
030c85d: 8350106: [PPC] Avoid ticks_unknown_not_Java AsyncGetCallTrace() if JavaFrameAnchor::_last_Java_pc not set
... and 30 more: https://git.openjdk.org/jdk/compare/088726238664985ebf2bc60deca96f22245e9ce3...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-12T10:22:57Z

@TheRealMDoerr @dbriemann Pushed as commit 4be502e.

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

dbriemann and others added 5 commits February 20, 2025 09:02

set some markers

e9ba782

switch bytecode counter to julong

6194f67

adjust assembler code to uint64 bytecode counter

6dc57b6

fix some print directives, add test 1st version

1891d8c

finish CountBytecodesTest

3cc63fb

bridgekeeper bot added the oca Needs verification of OCA signatory status label Feb 25, 2025

openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 25, 2025

dbriemann closed this Feb 25, 2025

dbriemann reopened this Feb 25, 2025

dbriemann changed the title ~~TODO title: upgrade CountBytecodes to 64 bit~~ 8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms Feb 25, 2025

dbriemann added 2 commits February 25, 2025 11:35

add copyright header and @bug number

2708e14

speed up runtime by running less bytecodes, add explanation

091cc88

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Feb 25, 2025

dbriemann added 3 commits February 25, 2025 14:11

add missing comma to copyright header

d58c550

remove bad header

4dbe796

formatting

714c580

myankelev reviewed Feb 25, 2025

View reviewed changes

address review comment, add back comma to copyright header

3165254

dbriemann added 2 commits February 26, 2025 08:58

fix x86 asm

fbe4067

remove auto included header

45699ec

lmesnik suggested changes Mar 5, 2025

View reviewed changes

bridgekeeper bot removed the oca Needs verification of OCA signatory status label Mar 5, 2025

bridgekeeper bot removed the oca-verify Needs verification of OCA signatory status label Mar 5, 2025

openjdk bot added the rfr Pull request is ready for review label Mar 5, 2025

dbriemann added 2 commits March 10, 2025 09:06

Merge branch 'master' into dlb/bytecode_counter_overflow

76bdd19

remove CountBytecodesTest from tier1

31a5215

TheRealMDoerr approved these changes Mar 11, 2025

View reviewed changes

shipilev approved these changes Mar 11, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 11, 2025

lmesnik approved these changes Mar 11, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 12, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 12, 2025

openjdk bot closed this Mar 12, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 12, 2025

dbriemann deleted the dlb/bytecode_counter_overflow branch March 12, 2025 10:23

		ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder("-Xint", "-XX:+CountBytecodes", "CountBytecodesTest", "test");
		OutputAnalyzer output = new OutputAnalyzer(pb.start());

8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms #23766

8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms #23766

Uh oh!

Conversation

dbriemann commented Feb 25, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 25, 2025

Uh oh!

openjdk bot commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 25, 2025

Uh oh!

dbriemann commented Feb 25, 2025

Uh oh!

bridgekeeper bot commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealMDoerr commented Feb 25, 2025

Uh oh!

RealFYang commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

offamitkumar commented Feb 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TheRealMDoerr left a comment

Choose a reason for hiding this comment

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmesnik left a comment

Choose a reason for hiding this comment

Uh oh!

dbriemann commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 12, 2025

Uh oh!

TheRealMDoerr commented Mar 12, 2025

Uh oh!

openjdk bot commented Mar 12, 2025

Uh oh!

openjdk bot commented Mar 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

dbriemann commented Feb 25, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 25, 2025 •

edited

Loading

RealFYang commented Feb 26, 2025 •

edited

Loading

mlbridge bot commented Mar 5, 2025 •

edited

Loading

dbriemann commented Mar 12, 2025 •

edited

Loading