8313435: Clean up unused default methods code #15095

coleenp · 2023-08-01T00:48:42Z

Default methods processing code has unused code (that gets -Wconversion warnings) from when it was used to create bridge (called overpass) method for an early implementation of generic reification in Hotspot.

This change removes unused bytecodeAssembler code and adds a check for methods and constant pool overflow.
Tested with tier1 and runtime/lambda-features tests.

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-8313435: Clean up unused default methods code (Enhancement - P4)

Reviewers

Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to c39d5291
Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to d1bcdc36

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15095

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-08-01T00:49:35Z

👋 Welcome back coleenp! 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 · 2023-08-01T00:51:04Z

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

hotspot-runtime

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 · 2023-08-01T00:55:11Z

Webrevs

kimbarrett

Looks good, except for a couple of oddly formatted parameter lists.

src/hotspot/share/classfile/bytecodeAssembler.hpp

src/hotspot/share/classfile/bytecodeAssembler.cpp

openjdk · 2023-08-01T15:10:12Z

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

8313435: Clean up unused default methods code

Reviewed-by: kbarrett, iklam

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

f66cd50: 8313564: Fix -Wconversion warnings in classfile code
e8a37b9: 8313248: C2: setScopedValueCache intrinsic exposes nullptr pre-values to store barriers
29f1d8e: 8313707: GHA: Bootstrap sysroots with --variant=minbase
61c58fd: 8312976: MatchResult produces StringIndexOutOfBoundsException for groups outside match
5d23295: 8313251: Add NativeLibraryLoad event
c4b8574: 8311938: Add default cups include location for configure on AIX
10a2605: 8294979: test/jdk/tools/jlink 3 test classes use ASM library
e8c325d: 8313394: Array Elements in OldObjectSample event has the incorrect description
d60352e: 8311006: missing @SInCE info in jdk.xml.dom
4577147: 8313712: [BACKOUT] 8313632: ciEnv::dump_replay_data use fclose
... and 39 more: https://git.openjdk.org/jdk/compare/6af0af593446bc33dc94bbf7334c325c4ac0ac0f...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.

coleenp · 2023-08-01T15:45:01Z

Thanks for reviewing, Kim.

dholmes-ora

Generally looks good but a couple of nits/queries.

Thanks

src/hotspot/share/classfile/bytecodeAssembler.cpp

src/hotspot/share/classfile/defaultMethods.cpp

…rflow an assert.

…methods error (aka overpass) methods.

iklam

Looks good to me. Thanks for making the BytecodeConstantPool::find_or_add() method fail early so we don't have potentially invalid bytecodes in the buffer.

dholmes-ora

Looks good except the new assert needs to be changed back I think.

src/hotspot/share/classfile/defaultMethods.cpp

dholmes-ora

I'm not at all sure that BytecodeConstantPool::find_or_add needs to throw, especially given that requires polluting so much code with TRAPS and CHECK - it looks really awful to see things like:

assem._new(errorName, CHECK_0));

Maybe we should be checking for sufficient space at a higher-level somewhere? I'm not familiar with how this code is actually used.

This seems to have strayed somewhat from the primary cleanup of unused code.

iklam · 2023-08-04T01:41:26Z

I'm not at all sure that BytecodeConstantPool::find_or_add needs to throw, especially given that requires polluting so much code with TRAPS and CHECK - it looks really awful to see things like:
assem._new(errorName, CHECK_0));
Maybe we should be checking for sufficient space at a higher-level somewhere? I'm not familiar with how this code is actually used.

This seems to have strayed somewhat from the primary cleanup of unused code.

It was my suggestion, because otherwise this code would be returning junk on overflow, leading to invalid code being stored in the buffer.

u2 BytecodeConstantPool::find_or_add(BytecodeCPEntry const& bcpe, TRAPS) {
  ...
  return static_cast<u2>(index + _orig->length());

Yes, there's code that checks after the fact that we have overflown the number for CP indices, and rejects the buffer. However, for security, it's better to not have invalid code in the first place. The places of invalid code generation and error detection are far apart, making the code hard to understand, and easy to mess up by future changes.

src/hotspot/share/classfile/bytecodeAssembler.cpp

dholmes-ora · 2023-08-04T03:50:30Z

It was my suggestion, because otherwise this code would be returning junk on overflow, leading to invalid code being stored in the buffer.

Okay but how do we arrive at that point? This is an internal implementation detail. We should be able to pre-check the capacity and reject higher-up rather than having the lowest method try to throw the exception (the lowest method should just assert). The profilteration of TRAPS and CHECK really looks ugly here.

coleenp · 2023-08-04T14:17:06Z

There's no ability to pre-check because we don't know how long the constant pool buffer is going to get until we generate the instructions that potentially add entries to the constant pool. Then we know.

dholmes-ora · 2023-08-07T04:23:26Z

There's no ability to pre-check because we don't know how long the constant pool buffer is going to get until we generate the instructions that potentially add entries to the constant pool. Then we know.

Doesn't assemble_method_error know how much CP space will be needed for the instructions it will generate?

coleenp · 2023-08-07T11:58:47Z

Doesn't assemble_method_error know how much CP space will be needed for the instructions it will generate?

Maybe it'll generate 4 or 5 constant pool entries but if it's not the first method error, it might not. It might share entries with a previously generated method error.

coleenp · 2023-08-07T12:00:20Z

There is no reason not to check for an unlikely case of overflow here. If we do have overflow, throwing an exception is better than asserting. This is a safety check to replace silently ignoring this overflow and returning the wrong value.

coleenp · 2023-08-08T12:11:02Z

Thanks Ioi and Kim for reviewing and David for your comments. There is a lot more opportunity to improve this code but it'll require a lot more study. This cleanup helps.
/integrate

openjdk · 2023-08-08T12:12:59Z

Going to push as commit 5c3041c.
Since your change was applied there have been 78 commits pushed to the master branch:

8752d49: 8313785: Fix -Wconversion warnings in prims code
41bdcde: 8313875: Use literals instead of static fields in java.util.Math: twoToTheDoubleScaleUp, twoToTheDoubleScaleDown
091e65e: 8313552: Fix -Wconversion warnings in JFR code
7e20952: 8140326: G1: Consider putting regions where evacuation failed into next collection set
28fd7a1: 8311179: Generational ZGC: gc/z/TestSmallHeap.java failed with OutOfMemoryError
a1115a7: 8312204: unexpected else with statement causes compiler crash
87a6acb: 8313792: Verify 4th party information in src/jdk.internal.le/share/legal/jline.md
87b08b6: 8307408: Some jdk/sun/tools/jhsdb tests don't pass test JVM args to the debuggee JVM
83edffa: 8309663: test fails "assert(check_alignment(result)) failed: address not aligned: 0x00000008baadbabe"
1da82a3: 8313702: Update IANA Language Subtag Registry to Version 2023-08-02
... and 68 more: https://git.openjdk.org/jdk/compare/6af0af593446bc33dc94bbf7334c325c4ac0ac0f...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-08T12:13:05Z

@coleenp Pushed as commit 5c3041c.

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

8313435: Clean up unused default methods code

c39d529

openjdk bot added the rfr Pull request is ready for review label Aug 1, 2023

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Aug 1, 2023

kimbarrett approved these changes Aug 1, 2023

View reviewed changes

src/hotspot/share/classfile/bytecodeAssembler.hpp Outdated Show resolved Hide resolved

src/hotspot/share/classfile/bytecodeAssembler.cpp Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Aug 1, 2023

Align parameters.

5b8e507

dholmes-ora reviewed Aug 2, 2023

View reviewed changes

coleenp added 2 commits August 2, 2023 12:33

Use USHRT_MAX instead of numeric limits template, and made method ove…

d787f46

…rflow an assert.

Check for constant pool overflow when appending entries from default …

d1bcdc3

…methods error (aka overpass) methods.

iklam approved these changes Aug 2, 2023

View reviewed changes

dholmes-ora suggested changes Aug 3, 2023

View reviewed changes

src/hotspot/share/classfile/defaultMethods.cpp Outdated Show resolved Hide resolved

src/hotspot/share/classfile/defaultMethods.cpp Show resolved Hide resolved

Make assert an exception.

559645d

dholmes-ora suggested changes Aug 4, 2023

View reviewed changes

iklam reviewed Aug 4, 2023

View reviewed changes

src/hotspot/share/classfile/bytecodeAssembler.cpp Outdated Show resolved Hide resolved

Change to checked_cast<>

124d2aa

openjdk bot added the integrated Pull request has been integrated label Aug 8, 2023

openjdk bot closed this Aug 8, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 8, 2023

coleenp deleted the default-methods branch August 8, 2023 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8313435: Clean up unused default methods code #15095

8313435: Clean up unused default methods code #15095

coleenp commented Aug 1, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 1, 2023

openjdk bot commented Aug 1, 2023

mlbridge bot commented Aug 1, 2023 •

edited

Loading

kimbarrett left a comment

openjdk bot commented Aug 1, 2023 •

edited

Loading

coleenp commented Aug 1, 2023

dholmes-ora left a comment

iklam left a comment

dholmes-ora left a comment

dholmes-ora left a comment •

edited

Loading

iklam commented Aug 4, 2023

dholmes-ora commented Aug 4, 2023

coleenp commented Aug 4, 2023

dholmes-ora commented Aug 7, 2023

coleenp commented Aug 7, 2023

coleenp commented Aug 7, 2023

coleenp commented Aug 8, 2023

openjdk bot commented Aug 8, 2023

openjdk bot commented Aug 8, 2023

8313435: Clean up unused default methods code #15095

8313435: Clean up unused default methods code #15095

Conversation

coleenp commented Aug 1, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 1, 2023

openjdk bot commented Aug 1, 2023

mlbridge bot commented Aug 1, 2023 • edited Loading

Webrevs

kimbarrett left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 1, 2023 • edited Loading

coleenp commented Aug 1, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

iklam left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment • edited Loading

Choose a reason for hiding this comment

iklam commented Aug 4, 2023

dholmes-ora commented Aug 4, 2023

coleenp commented Aug 4, 2023

dholmes-ora commented Aug 7, 2023

coleenp commented Aug 7, 2023

coleenp commented Aug 7, 2023

coleenp commented Aug 8, 2023

openjdk bot commented Aug 8, 2023

openjdk bot commented Aug 8, 2023

coleenp commented Aug 1, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 1, 2023 •

edited

Loading

openjdk bot commented Aug 1, 2023 •

edited

Loading

dholmes-ora left a comment •

edited

Loading