8254269: simplify Node::disconnect_inputs #589

navyxliu · 2020-10-10T14:53:23Z

Progress

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

Testing

	Linux x64	Windows x64	macOS x64
Build	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

JDK-8254269: simplify Node::disconnect_inputs

Reviewers

Claes Redestad (@cl4es - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Download

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

bridgekeeper · 2020-10-10T14:54:17Z

👋 Welcome back xliu! 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-10-10T14:55:39Z

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

hotspot-compiler

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-10-10T14:59:12Z

Webrevs

navyxliu · 2020-10-10T15:05:55Z

/summary Node::disconnect_inputs cuts off all input edges without exception.

openjdk · 2020-10-10T15:06:48Z

@navyxliu Setting summary to Node::disconnect_inputs cuts off all input edges without exception.

cl4es

Nice cleanup! A few suggestions inline, but looks good as-is, too.

src/hotspot/share/opto/node.cpp

openjdk · 2020-10-10T15:16:04Z

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

8254269: simplify Node::disconnect_inputs

Node::disconnect_inputs cuts off all input edges without exception.

Reviewed-by: redestad, 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 26 new commits pushed to the master branch:

d7128e7: 8254090: Collectors.toUnmodifiableList exposes shared secret
df1f132: 8253563: Change sun.security.jca.Providers.threadLists to be ThreadLocal
c7f0064: 8253899: Make IsClassUnloadingEnabled signature match specification
aad3cf4: 8254234: Add test library stream object builder
4184959: 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher.
05459df: 8253765: C2: Control randomization in StressLCM and StressGCM
6620b61: 8254573: Shenandoah: Streamline/inline native-LRB entry point
a6c23b7: 8253923: C2 doesn't always run loop opts for compilations that include loops
dfe8ba6: 8254320: Shenandoah: C2 native LRB should activate for non-cset objects
295a44a: 8254558: Remove unimplemented Arguments::do_pd_flag_adjustments
... and 16 more: https://git.openjdk.java.net/jdk/compare/ec41046c5ce7077eebf4a3c265f79c7fba33d916...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 (@cl4es, @vnkozlov) 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).

Optimize it for the precedence loop. because there's no null between 2 non-null precedences, disconnect_inputs can break at a null value.

cl4es · 2020-10-11T13:06:57Z

I think the changes in b8a7275 might be fine, but they are more subtle than the cleanup in the preceding version and I'm not comfortable reviewing them. I think it needs a more thorough examination of if those optimizations always hold. Could probably use some more asserts, a few tests and lift the documented assumption of the structure of the input to a more visible place. My suggestion is to push this ahead with the straightforward cleanup you had in 4a9d803 and file a follow-up RFE for the other optimization.

src/hotspot/share/opto/node.cpp

navyxliu · 2020-10-11T20:59:16Z

I think the changes in b8a7275 might be fine, but they are more subtle than the cleanup in the preceding version and I'm not comfortable reviewing them. I think it needs a more thorough examination of if those optimizations always hold. Could probably use some more asserts, a few tests and lift the documented assumption of the structure of the input to a more visible place. My suggestion is to push this ahead with the straightforward cleanup you had in 4a9d803 and file a follow-up RFE for the other optimization.

yes, thank for the advice.

src/hotspot/share/opto/node.cpp

navyxliu · 2020-10-12T04:54:57Z

I filed JDK-8254369 for the bug. Let's keep this PR as a clean-up.

navyxliu · 2020-10-12T17:26:45Z

/integrate

openjdk · 2020-10-12T17:28:14Z

@navyxliu
Your change (at version 5c6bb10) is now ready to be sponsored by a Committer.

vnkozlov · 2020-10-12T19:53:02Z

/sponsor

openjdk · 2020-10-12T19:54:52Z

@vnkozlov @navyxliu Since your change was applied there have been 26 commits pushed to the master branch:

d7128e7: 8254090: Collectors.toUnmodifiableList exposes shared secret
df1f132: 8253563: Change sun.security.jca.Providers.threadLists to be ThreadLocal
c7f0064: 8253899: Make IsClassUnloadingEnabled signature match specification
aad3cf4: 8254234: Add test library stream object builder
4184959: 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher.
05459df: 8253765: C2: Control randomization in StressLCM and StressGCM
6620b61: 8254573: Shenandoah: Streamline/inline native-LRB entry point
a6c23b7: 8253923: C2 doesn't always run loop opts for compilations that include loops
dfe8ba6: 8254320: Shenandoah: C2 native LRB should activate for non-cset objects
295a44a: 8254558: Remove unimplemented Arguments::do_pd_flag_adjustments
... and 16 more: https://git.openjdk.java.net/jdk/compare/ec41046c5ce7077eebf4a3c265f79c7fba33d916...master

Your commit was automatically rebased without conflicts.

Pushed as commit bff586f.

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

8254269: simplify Node::disconnect_inputs

4a9d803

openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org rfr Pull request is ready for review labels Oct 10, 2020

cl4es approved these changes Oct 10, 2020

View reviewed changes

src/hotspot/share/opto/node.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/node.cpp Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Oct 10, 2020

8254269: simplify Node::disconnect_inputs

b8a7275

Optimize it for the precedence loop. because there's no null between 2 non-null precedences, disconnect_inputs can break at a null value.

vnkozlov suggested changes Oct 11, 2020

View reviewed changes

src/hotspot/share/opto/node.cpp Show resolved Hide resolved

src/hotspot/share/opto/node.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/node.cpp Show resolved Hide resolved

vnkozlov suggested changes Oct 11, 2020

View reviewed changes

src/hotspot/share/opto/node.cpp Outdated Show resolved Hide resolved

8254269: simplify Node::disconnect_inputs

5c6bb10

cl4es approved these changes Oct 12, 2020

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 12, 2020

vnkozlov approved these changes Oct 12, 2020

View reviewed changes

openjdk bot closed this Oct 12, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 12, 2020

8254269: simplify Node::disconnect_inputs #589

8254269: simplify Node::disconnect_inputs #589

navyxliu commented Oct 10, 2020 •

edited by openjdk bot

bridgekeeper bot commented Oct 10, 2020

openjdk bot commented Oct 10, 2020

mlbridge bot commented Oct 10, 2020 •

edited

navyxliu commented Oct 10, 2020

openjdk bot commented Oct 10, 2020

cl4es left a comment

openjdk bot commented Oct 10, 2020 •

edited

cl4es commented Oct 11, 2020

navyxliu commented Oct 11, 2020

navyxliu commented Oct 12, 2020

navyxliu commented Oct 12, 2020

openjdk bot commented Oct 12, 2020

vnkozlov commented Oct 12, 2020

openjdk bot commented Oct 12, 2020

8254269: simplify Node::disconnect_inputs #589

8254269: simplify Node::disconnect_inputs #589

Conversation

navyxliu commented Oct 10, 2020 • edited by openjdk bot

Progress

Testing

Issue

Reviewers

Download

bridgekeeper bot commented Oct 10, 2020

openjdk bot commented Oct 10, 2020

mlbridge bot commented Oct 10, 2020 • edited

Webrevs

navyxliu commented Oct 10, 2020

openjdk bot commented Oct 10, 2020

cl4es left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 10, 2020 • edited

cl4es commented Oct 11, 2020

navyxliu commented Oct 11, 2020

navyxliu commented Oct 12, 2020

navyxliu commented Oct 12, 2020

openjdk bot commented Oct 12, 2020

vnkozlov commented Oct 12, 2020

openjdk bot commented Oct 12, 2020

navyxliu commented Oct 10, 2020 •

edited by openjdk bot

mlbridge bot commented Oct 10, 2020 •

edited

openjdk bot commented Oct 10, 2020 •

edited