Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines #3394

Closed
wants to merge 9 commits into from

Conversation

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Apr 8, 2021

The existing JRT_ENTRY (and related) macros require the function to which they are applied to declare a parameter "JavaThread* thread" which represents the current thread. These functions are all implicitly "traps" functions as they can result in exceptions, but they are not declared with TRAPS because the only caller of these functions is the runtime itself (via call_VM) and no callers need to be aware to use CHECK; further they need a JavaThread. So the macro declares the THREAD variable for use with other exception-producing functions and assigns it from "thread".

The majority of this change replaces the parameter name "thread" with "current" so that it is clear that we are always dealing with the current thread. This affects the entry functions as well as the functions called therefrom.

We can then also replace the use of "THREAD" with "current", in contexts that are not related to exception processing.

Some methods called by entry functions were declared to have both a "thread" parameter and a "TRAPS" parameter - with nothing to tell you these are always the same, current, thread. So the "thread" parameter is removed and replaced with a local variable "current" obtained from THREAD->as_Java_thread().

Some missing CHECK_ uses were added.

Testing:

  • tiers 1-3

Thanks,
David


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3394/head:pull/3394
$ git checkout pull/3394

Update a local copy of the PR:
$ git checkout pull/3394
$ git pull https://git.openjdk.java.net/jdk pull/3394/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3394

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3394.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 8, 2021

👋 Welcome back dholmes! 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.

Loading

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 8, 2021

/cc hotspot-runtime
/cc hotspot-compiler
/cc remove hotspot

Loading

@dholmes-ora dholmes-ora marked this pull request as ready for review Apr 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

@dholmes-ora
The hotspot-runtime label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 8, 2021

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

@dholmes-ora
The hotspot-compiler label was successfully added.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

@dholmes-ora The hotspot label was not set.

Loading

coleenp
coleenp approved these changes Apr 8, 2021
Copy link
Contributor

@coleenp coleenp left a comment

I think substituting "JavaThread* thread" for "JavaThread* current" is a good change and convention that makes the code more clear, so worth the dull code review and diffs.

Loading

src/hotspot/share/c1/c1_Runtime1.cpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/c1/c1_Runtime1.cpp Show resolved Hide resolved
Loading
src/hotspot/share/c1/c1_Runtime1.cpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/interpreter/interpreterRuntime.cpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/interpreter/interpreterRuntime.cpp Outdated Show resolved Hide resolved
Loading
@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

@dholmes-ora 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:

8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Reviewed-by: coleenp, hseigel, iklam, kvn

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

Loading

@openjdk openjdk bot added the ready label Apr 8, 2021
@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 8, 2021

Looks like I missed some definitions that aren't included in our test builds but have been found via GHA builds. I will rectify those and update.

Loading

hseigel
hseigel approved these changes Apr 9, 2021
Copy link
Member

@hseigel hseigel left a comment

Nice big cleanup! LGTM
Harold

Loading

iklam
iklam approved these changes Apr 12, 2021
Copy link
Member

@iklam iklam left a comment

LGTM

Loading

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 12, 2021

Thanks for the reviews @coleenp , @hseigel and @iklam .

Can someone from compiler team please take a look and give the okay? (I know things are a bit busy at the moment.)

Thanks,
David

Loading

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 14, 2021

Repeating this as the bots failed to send out an email.

Thanks for the reviews @coleenp , @hseigel and @iklam .

Can someone from compiler team please take a look and give the okay? (I know things are a bit busy at the moment.)

Thanks,
David

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Why you kept JavaThread *thread parameter in some methods and renamed in others in deoptimization.* files.

Loading

RegisterMap reg_map(thread, false);
frame runtime_frame = thread->last_frame();
static void deopt_caller(JavaThread* current) {
if ( !caller_is_deopted(current)) {
Copy link
Contributor

@vnkozlov vnkozlov Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove space after ( ?

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 15, 2021

I also don't see changes in src/hotspot/share/ci/. Is code there okay?

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 15, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Vladimir,

Thanks for looking at this.

On 15/04/2021 9:52 am, Vladimir Kozlov wrote:

On Fri, 9 Apr 2021 05:08:37 GMT, David Holmes <dholmes at openjdk.org> wrote:

The existing JRT_ENTRY (and related) macros require the function to which they are applied to declare a parameter "JavaThread* thread" which represents the current thread. These functions are all implicitly "traps" functions as they can result in exceptions, but they are not declared with TRAPS because the only caller of these functions is the runtime itself (via call_VM) and no callers need to be aware to use CHECK; further they need a JavaThread. So the macro declares the THREAD variable for use with other exception-producing functions and assigns it from "thread".

The majority of this change replaces the parameter name "thread" with "current" so that it is clear that we are always dealing with the current thread. This affects the entry functions as well as the functions called therefrom.

We can then also replace the use of "THREAD" with "current", in contexts that are not related to exception processing.

Some methods called by entry functions were declared to have both a "thread" parameter and a "TRAPS" parameter - with nothing to tell you these are always the same, current, thread. So the "thread" parameter is removed and replaced with a local variable "current" obtained from THREAD->as_Java_thread().

Some missing CHECK_ uses were added.

Testing:
- tiers 1-3

Thanks,
David

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Fix search&replace mistake

Why you kept `JavaThread *thread` parameter in some methods and renamed in others in deoptimization.* files.

I only changed those parameters involved with JRT_* routines, as those
can obviously be seen to be the current thread. For other code in that
file it probably deals with the current thread, but that isn't
necessarily obvious and would need more rigorous checking. That is
beyond the scope of the current set of changes and would need a future RFE.

Similarly for the lack of changes in the ci files - if they weren't
affected by the JRT changes they weren't changed.

src/hotspot/share/c1/c1_Runtime1.cpp line 178:

176: // Stress deoptimization
177: static void deopt_caller(JavaThread* current) {
178: if ( !caller_is_deopted(current)) {

Can you remove space after `( `?

Sure. :)

Thanks,
David
-----

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 15, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Vladimir,

Thanks for looking at this.

On 15/04/2021 9:52 am, Vladimir Kozlov wrote:

On Fri, 9 Apr 2021 05:08:37 GMT, David Holmes <dholmes at openjdk.org> wrote:

The existing JRT_ENTRY (and related) macros require the function to which they are applied to declare a parameter "JavaThread* thread" which represents the current thread. These functions are all implicitly "traps" functions as they can result in exceptions, but they are not declared with TRAPS because the only caller of these functions is the runtime itself (via call_VM) and no callers need to be aware to use CHECK; further they need a JavaThread. So the macro declares the THREAD variable for use with other exception-producing functions and assigns it from "thread".

The majority of this change replaces the parameter name "thread" with "current" so that it is clear that we are always dealing with the current thread. This affects the entry functions as well as the functions called therefrom.

We can then also replace the use of "THREAD" with "current", in contexts that are not related to exception processing.

Some methods called by entry functions were declared to have both a "thread" parameter and a "TRAPS" parameter - with nothing to tell you these are always the same, current, thread. So the "thread" parameter is removed and replaced with a local variable "current" obtained from THREAD->as_Java_thread().

Some missing CHECK_ uses were added.

Testing:
- tiers 1-3

Thanks,
David

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Fix search&replace mistake

Why you kept `JavaThread *thread` parameter in some methods and renamed in others in deoptimization.* files.

I only changed those parameters involved with JRT_* routines, as those
can obviously be seen to be the current thread. For other code in that
file it probably deals with the current thread, but that isn't
necessarily obvious and would need more rigorous checking. That is
beyond the scope of the current set of changes and would need a future RFE.

Similarly for the lack of changes in the ci files - if they weren't
affected by the JRT changes they weren't changed.

src/hotspot/share/c1/c1_Runtime1.cpp line 178:

176: // Stress deoptimization
177: static void deopt_caller(JavaThread* current) {
178: if ( !caller_is_deopted(current)) {

Can you remove space after `( `?

Sure. :)

Thanks,
David
-----

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Okay.

Loading

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 15, 2021

Thanks for the review @vnkozlov !

David

Loading

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 15, 2021

/integrate

Loading

@openjdk openjdk bot closed this Apr 15, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 15, 2021

@dholmes-ora Pushed as commit 79bff21.

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

Loading

@dholmes-ora dholmes-ora deleted the 8263709 branch Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment