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

8258961: move some fields of SafePointNode from public to protected #1899

Closed
wants to merge 5 commits into from

Conversation

@navyxliu
Copy link
Contributor

@navyxliu navyxliu commented Dec 28, 2020

Declare VMStructs is a friend of SafePointNode so we can move some fields from public zone to protected zone.
Clean up the forward declaration section in callnode.hpp. Lock/Unlock Nodes are subclasses of AbstractLockNode.

/cc hotspot-compiler


Progress

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

Issue

  • JDK-8258961: move some fields of SafePointNode from public to protected

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 28, 2020

👋 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 28, 2020

@navyxliu
The hotspot-compiler label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 28, 2020

Loading

src/hotspot/share/opto/callnode.hpp Outdated Show resolved Hide resolved
Loading
Copy link
Contributor

@vnkozlov vnkozlov left a comment

Also update last copyright year in changed files.

Loading

src/hotspot/share/runtime/vmStructs.cpp Outdated Show resolved Hide resolved
Loading
devirtualize SafePointNode::jvms(). We can't hide the member variable _jvms
because it is exposed to HotSpot Servicability agent. keeping the qualifier
const for the same resaon.
Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 5, 2021

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

8258961: move some fields of SafePointNode from public to protected

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

  • 6b4732f: 8259679: GitHub actions should use MSVC 14.28
  • 061ffc4: 8249245: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
  • e60c992: 8259849: Shenandoah: Rename store-val to IU-barrier
  • db9c114: 7146776: deadlock between URLStreamHandler.getHostAddress and file.Handler.openconnection
  • 61292be: 8259681: Remove the Marlin rendering engine (single-precision)
  • ff275b3: 8259403: Zero: crash with NULL MethodHandle receiver
  • e93f08e: 8074101: Add verification that all tasks are actually claimed during roots processing
  • 917f7e9: 8259650: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button"
  • 3f19ef6: 8202880: Test javax/swing/JPopupMenu/8075063/ContextMenuScrollTest.java fails
  • 68cf65d: 8023980: JCE doesn't provide any class to handle RSA private key in PKCS#1
  • ... and 212 more: https://git.openjdk.java.net/jdk/compare/8b4549773b649a33f8f6795cd054b4bb2ec607ec...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 (@TobiHartmann, @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).

Loading

@openjdk openjdk bot added the ready label Jan 5, 2021
@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Jan 5, 2021

I investigate the 3 failures on x86 host. they have been fixed in JDK-8258703 (committed yesterday).
Presumably, the regression tests are clear. /integrate.

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Jan 5, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Jan 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 5, 2021

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

Loading

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Jan 6, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 6, 2021

@TobiHartmann This PR has not yet been marked as ready for integration.

Loading

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Jan 6, 2021

@navyxliu There is a title mismatch between the PR and the JBS issue that you need to fix before integration.

Loading

@navyxliu navyxliu changed the title 8258961: devirtualize SafePointNode::jvms() and hide out set_jvms() 8258961: devirtualize SafePointNode::jvms() Jan 6, 2021
@openjdk openjdk bot added the ready label Jan 6, 2021
@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Jan 6, 2021

Yes, I did modify the JBS title. I found it's hard to hide out _jvms because offset_of(klass, field) needs to access the field.

I plan to see if we can use template metaprogramming or constexpr to workaround it(https://gist.github.com/graphitemaster/494f21190bb2c63c5516). I will file a separate issue if I figure out how.

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Jan 8, 2021

I found Node::jvms() is virtual, so it's useless to remove keyword "virtual" in SafePointNode::jvms(). I need another revision to fix it.

Paul asked me to hide away those public fields into protected. I discover a straightforward solution -- just declare friend class VMStructs. Other classes do that.

Loading

Xin Liu added 2 commits Jan 9, 2021
remove the virtual qualifier from Node::jvms(). check its type is SafePointNode and
MachSafePointNode instead.

This patch also move some fiels to proctected zone and declare VMStructs is a friend.
revert code change to devirtualize Node::jvms().
There are 3 nodes override jvms() --  SafePointNode, MachSafePointNode and
MachHalt. It does not make sense to devirtualize it because we have to do
non-trivial dispatch based on types in Node::jvms(). It is not worth it.
@openjdk openjdk bot removed the ready label Jan 15, 2021
@navyxliu navyxliu changed the title 8258961: devirtualize SafePointNode::jvms() 8258961: move some fields of SafePointNode from public to protected Jan 15, 2021
@openjdk openjdk bot added the ready label Jan 15, 2021
@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Jan 15, 2021

I took back the devirtual attempt because it's not worth it. there're 3 nodes override Node::jvms().
non-virtual jvms() looks more expensive to access vtable.

JVMState* Node::jvms() const {
  if (is_SafePoint()) {
    return as_SafePoint()->jvms();
  } else if (is_MachSafePoint()) {
    return as_MachSafePoint()->jvms();
  } else if (is_MachHalt()) {
   return as_MachHalt()->jvms();
  }
  return nullptr;
}

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Jan 15, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Jan 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 15, 2021

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

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Jan 15, 2021

hello, @TobiHartmann ,
Could you take a look at this PR again? I've corrected title and description to reflect what this patch does.

Loading

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks reasonable to me but @vnkozlov should also review this again.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Looks good.

Latest version passed hs-tier1-3 testing.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Jan 18, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 18, 2021

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

  • 6b4732f: 8259679: GitHub actions should use MSVC 14.28
  • 061ffc4: 8249245: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
  • e60c992: 8259849: Shenandoah: Rename store-val to IU-barrier
  • db9c114: 7146776: deadlock between URLStreamHandler.getHostAddress and file.Handler.openconnection
  • 61292be: 8259681: Remove the Marlin rendering engine (single-precision)
  • ff275b3: 8259403: Zero: crash with NULL MethodHandle receiver
  • e93f08e: 8074101: Add verification that all tasks are actually claimed during roots processing
  • 917f7e9: 8259650: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button"
  • 3f19ef6: 8202880: Test javax/swing/JPopupMenu/8075063/ContextMenuScrollTest.java fails
  • 68cf65d: 8023980: JCE doesn't provide any class to handle RSA private key in PKCS#1
  • ... and 212 more: https://git.openjdk.java.net/jdk/compare/8b4549773b649a33f8f6795cd054b4bb2ec607ec...master

Your commit was automatically rebased without conflicts.

Pushed as commit 533a2d3.

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

Loading

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Jan 18, 2021

Thanks for all reviewers.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants