8256754: Deoptimization::revoke_for_object_deoptimization: stack processing start call is redundant #1381

reinrich · 2020-11-23T11:40:49Z

This is a XS clean-up of Deoptimization::revoke_for_object_deoptimization() which removes the StackWatermarkSet::start_processing() call.

This is correct because all paths leading to revoke_for_object_deoptimization() are equipped with a KeepStackGCProcessedMark.

Call Tree:

StackWatermarkSet::start_processing(JavaThread *, enum StackWatermarkKind) : void
    Deoptimization::revoke_for_object_deoptimization(JavaThread *, frame, RegisterMap *, JavaThread *) : void
        Deoptimization::deoptimize_objects_internal(JavaThread *, GrowableArray<compiledVFrame *> *, bool &) : bool
            EscapeBarrier::deoptimize_objects_internal(JavaThread *, intptr_t *) : bool
                EscapeBarrier::deoptimize_objects_all_threads() : bool                     // has KeepStackGCProcessedMark
                EscapeBarrier::deoptimize_objects(intptr_t *) : bool
                    EscapeBarrier::deoptimize_objects(int, int) : bool                     // has KeepStackGCProcessedMark

Testing: hotspot_serviceability, jdk_svc, jdk_jdi, vmTestbase_nsk_jdi, vmTestbase_nsk_jvmti, vmTestbase_nsk_jdwp with -XX:+UseZGC.

Progress

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

Testing

	Linux aarch64	Linux arm	Linux ppc64le	Linux s390x	Linux x64	Linux x86	Windows x64	macOS x64
Build	✔️ (1/1 passed)	✔️ (1/1 passed)	✔️ (1/1 passed)	✔️ (1/1 passed)	✔️ (6/6 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)					✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

JDK-8256754: Deoptimization::revoke_for_object_deoptimization: stack processing start call is redundant

Reviewers

Dean Long (@dean-long - Reviewer)
Erik Österlund (@fisk - Reviewer) ⚠️ Review applies to f09aaf9

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1381/head:pull/1381
$ git checkout pull/1381

…essing start call is redundant

bridgekeeper · 2020-11-23T11:41:32Z

👋 Welcome back rrich! 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 · 2020-11-23T11:42:40Z

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

mlbridge · 2020-11-23T15:30:00Z

Webrevs

01: Full - Incremental (7f61dbe)
00: Full (f09aaf9)

dean-long

The justification for removing this code is that all callers use KeepStackGCProcessedMark. Is there an assert you can add in place of the removed code that checks this invariant?

fisk

Looks good.

openjdk · 2020-11-24T08:44:46Z

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

8256754: Deoptimization::revoke_for_object_deoptimization: stack processing start call is redundant

Reviewed-by: dlong, eosterlund

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

3c230b8: 8256993: Clarify Package::isSealed javadoc about package sealing vs sealed class or interface
1b7a61f: 8254999: Move G1RemSetSamplingTask to more appropriate location
695117f: 8255479: [aarch64] assert(src->section_index_of(target) == CodeBuffer::SECT_NONE) failed: sanity
dbfeb90: 8243559: Remove root certificates with 1024-bit keys
2a1e9be: 8256364: vmTestbase/nsk/jvmti/scenarios/capability/CM01/cm01t002 failed with "assert(handle != __null) failed: JNI handle should not be null"
f1d6e8d: 8256387: Unexpected result if patching an entire instruction on AArch64
bd14274: 8256480: Refactor ObjectInputStream field reader implementation
1c4c99e: 8256823: C2 compilation fails with "assert(isShiftCount(imm8 >> 1)) failed: illegal shift count"
3b3e90e: 8256924: ppc, ppcle, s390: JVM crashes at VM init after JDK-8254231
ff6df15: 8256801: tools/jpackage/share/FileAssociationsTest.java#id0 failed unpack.bat with "Exit code: 1603"
... and 32 more: https://git.openjdk.java.net/jdk/compare/53f38353e0d07b8b4435ed435390ed01e9b24d59...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.

reinrich · 2020-11-24T11:28:51Z

Thanks for having a look @dean-long

The justification for removing this code is that all callers use KeepStackGCProcessedMark. Is there an assert you can add in place of the removed code that checks this invariant?

I don't think there is an assert I could use. I added code that allows for asserting this. Let me know which version you prefer.

reinrich · 2020-11-24T11:31:54Z

Looks good.

Thank you @fisk
Note that I added code to assert that the remote stack is kept gc processed. Just in case you have comments on that.

dean-long · 2020-11-24T23:15:27Z

I like the version with the assert.

reinrich · 2020-11-25T08:59:34Z

I like the version with the assert.

Good. Thanks for reviewing!

reinrich · 2020-11-30T08:36:34Z

/integrate

openjdk · 2020-11-30T08:37:31Z

@reinrich Since your change was applied there have been 104 commits pushed to the master branch:

738efea: 8248564: JFR: Remote Recording Stream
9bcd269: 8257221: C2: RegMask::is_bound_set split set handling broken since JDK-8221404
222e943: 8257238: Cleanup include directives for precompiled.hpp
fdee70d: 8257237: Cleanup unused imports in the SunJSSE provider implementation
816e8f8: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
c5d9507: 8257220: [JVMCI] option validation should not result in a heavy-weight VM crash
c2af27b: 8257148: Remove obsolete code in AWTView.m
87f37aa: 8256726: Zero: print proper message in ArrayIndexOutOfBoundException
f4c15a9: 8256832: Zero: micro-optimize safepoint handling after JDK-8255384
04eecf0: 8257233: Windows x86 build is broken by JDK-8252684
... and 94 more: https://git.openjdk.java.net/jdk/compare/53f38353e0d07b8b4435ed435390ed01e9b24d59...master

Your commit was automatically rebased without conflicts.

Pushed as commit e77aed6.

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

8256754: Deoptimization::revoke_for_object_deoptimization: stack proc…

f09aaf9

…essing start call is redundant

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 23, 2020

reinrich marked this pull request as ready for review November 23, 2020 15:26

openjdk bot added the rfr Pull request is ready for review label Nov 23, 2020

dean-long suggested changes Nov 23, 2020

View reviewed changes

fisk approved these changes Nov 24, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 24, 2020

Assert stack is kept gc processed.

7f61dbe

dean-long approved these changes Nov 24, 2020

View reviewed changes

openjdk bot closed this Nov 30, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8256754: Deoptimization::revoke_for_object_deoptimization: stack processing start call is redundant #1381

8256754: Deoptimization::revoke_for_object_deoptimization: stack processing start call is redundant #1381

reinrich commented Nov 23, 2020 •

edited by openjdk bot

bridgekeeper bot commented Nov 23, 2020

openjdk bot commented Nov 23, 2020

mlbridge bot commented Nov 23, 2020 •

edited

dean-long left a comment

fisk left a comment

openjdk bot commented Nov 24, 2020 •

edited

reinrich commented Nov 24, 2020

reinrich commented Nov 24, 2020

dean-long commented Nov 24, 2020

reinrich commented Nov 25, 2020

reinrich commented Nov 30, 2020

openjdk bot commented Nov 30, 2020

8256754: Deoptimization::revoke_for_object_deoptimization: stack processing start call is redundant #1381

8256754: Deoptimization::revoke_for_object_deoptimization: stack processing start call is redundant #1381

Conversation

reinrich commented Nov 23, 2020 • edited by openjdk bot

Progress

Testing

Issue

Reviewers

Download

bridgekeeper bot commented Nov 23, 2020

openjdk bot commented Nov 23, 2020

mlbridge bot commented Nov 23, 2020 • edited

Webrevs

dean-long left a comment

Choose a reason for hiding this comment

fisk left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 24, 2020 • edited

reinrich commented Nov 24, 2020

reinrich commented Nov 24, 2020

dean-long commented Nov 24, 2020

reinrich commented Nov 25, 2020

reinrich commented Nov 30, 2020

openjdk bot commented Nov 30, 2020

reinrich commented Nov 23, 2020 •

edited by openjdk bot

mlbridge bot commented Nov 23, 2020 •

edited

openjdk bot commented Nov 24, 2020 •

edited