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

8264742: member variable _monitor of MonitorLocker is redundant #3350

Closed
wants to merge 1 commit into from

Conversation

@navyxliu
Copy link
Contributor

@navyxliu navyxliu commented Apr 6, 2021

MonitorLocker is a subclass of MutexLocker. The member variable _monitor
has the same value of _mutex in its base class.


Progress

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

Issue

  • JDK-8264742: member variable _monitor of MonitorLocker is redundant

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3350

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 6, 2021

👋 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 openjdk bot added the rfr label Apr 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 6, 2021

@navyxliu 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
Copy link

@mlbridge mlbridge bot commented Apr 6, 2021

Webrevs

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 6, 2021

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

Hi Xin,

On 6/04/2021 2:11 pm, Xin Liu wrote:

MonitorLocker is a subclass of MutexLocker. The member variable _monitor
has the same value of _mutex in its base class.

Yes it does, but it has a different type obviously. Do your as_monitor
calls actually get completely elided by the compiler? If so then this
change seems okay, but otherwise I prefer to save any runtime overhead
by using an extra word of stack.

Thanks,
David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 6, 2021

Mailing list message from Liu, Xin on hotspot-runtime-dev:

Hi, David,

Yes, as_monitor() gets completely elided by gcc.

I inspect function VMThread::wait_for_vm_thread_exit() as a sample. In release build, all member functions of
MonitorLocker are successfully inlined by GCC 10.2.1. The code is exactly same as before. It's because optimizing
compiler can perform redundant store elimination with alias analysis. I don't think this PR will help much on
performance in release binaries if they built by GCC/LLVM.

In fast-debug build, as_monitor() is inlined and deleted. A ctor MonitorLocker(Monitor*, Mutex::SafepointCheckFlag)
isn't inlined. I compared the generated code. The code size reduces from 272 bytes to 238 in x86.
After applying my patch, this ctor can save 1 caller-saved register and a store instruction which is generated from _monitor.
I have uploaded comparison to https://bugs.openjdk.java.net/browse/JDK-8264742.

Conclusion:
as_monitor() doesn't bring extra cost. Lacking of aggressive compiler optimizations, this patch can save one machine-word
on stack and a store for each MonitorLocker object. At least, it can bring a little bit performance for debug builds.

Thanks,
--lx

?On 4/5/21, 9:24 PM, "David Holmes" <david.holmes at oracle.com> wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.

Hi Xin,

On 6/04/2021 2:11 pm, Xin Liu wrote:
> MonitorLocker is a subclass of MutexLocker. The member variable _monitor
> has the same value of _mutex in its base class.

Yes it does, but it has a different type obviously. Do your as_monitor
calls actually get completely elided by the compiler? If so then this
change seems okay, but otherwise I prefer to save any runtime overhead
by using an extra word of stack.

Thanks,
David

> -------------
>
> Commit messages:
> - member variable _monitor of MonitorLocker is redundant
>
> Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3350&range=00
> Issue: https://bugs.openjdk.java.net/browse/JDK-8264742
> Stats: 15 lines in 1 file changed: 5 ins; 1 del; 9 mod
> Fetch: git fetch https://git.openjdk.java.net/jdk pull/3350/head:pull/3350
>
>

@coleenp
coleenp approved these changes Apr 6, 2021
Copy link

@coleenp coleenp left a comment

This is ok. I was worried about the cast defeating type safety but as long as as_monitor is in MonitorLocker, that's fine.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 6, 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:

8264742: member variable _monitor of MonitorLocker is redundant

Reviewed-by: coleenp, dholmes, phh

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

  • 7a99a98: 8262316: Reducing locks in RSA Blinding
  • d3fdd73: 8264173: [s390] Improve Hardware Feature Detection And Reporting
  • 9d65039: 8263984: Invalidate printServices when there are no printers
  • adb860e: 8255800: Raster creation methods need some specification clean up
  • eab8455: 8261137: Optimization of Box nodes in uncommon_trap
  • 92fad1b: 8264680: Use the blessed modifier order in java.desktop
  • 17202c8: 8264748: Do not include arguments.hpp from compilerDefinitions.hpp
  • c3abdc9: 8264797: Do not include klassVtable.hpp from instanceKlass.hpp
  • eb5c097: 8262389: Use permitted_enctypes if default_tkt_enctypes or default_tgs_enctypes is not present
  • bfb034a: 8264524: jdk/internal/platform/docker/TestDockerMemoryMetrics.java fails due to swapping not working
  • ... and 10 more: https://git.openjdk.java.net/jdk/compare/ff223530b66a83fb77c477da97a2dd60df448ec8...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 (@coleenp, @dholmes-ora, @phohensee) 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).

@openjdk openjdk bot added the ready label Apr 6, 2021
@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Apr 6, 2021

This is ok. I was worried about the cast defeating type safety but as long as as_monitor is in MonitorLocker, that's fine.

hi, Coleen,

Thank you for reviewing this patch!
FWIW, the type of _mutex is Monitor* in MonitorLocker object. static_cast<Monitor*> just makes use that. Arbitrary casting is error-prone, but this is not the case.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Thanks for the additional investigation of the code generation - much appreciated.

Changes look good.

Thanks,
David

@navyxliu
Copy link
Contributor Author

@navyxliu navyxliu commented Apr 7, 2021

/integrate

@openjdk openjdk bot added the sponsor label Apr 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

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

@phohensee
Copy link
Member

@phohensee phohensee commented Apr 7, 2021

/sponsor

@openjdk openjdk bot closed this Apr 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

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

  • 7a99a98: 8262316: Reducing locks in RSA Blinding
  • d3fdd73: 8264173: [s390] Improve Hardware Feature Detection And Reporting
  • 9d65039: 8263984: Invalidate printServices when there are no printers
  • adb860e: 8255800: Raster creation methods need some specification clean up
  • eab8455: 8261137: Optimization of Box nodes in uncommon_trap
  • 92fad1b: 8264680: Use the blessed modifier order in java.desktop
  • 17202c8: 8264748: Do not include arguments.hpp from compilerDefinitions.hpp
  • c3abdc9: 8264797: Do not include klassVtable.hpp from instanceKlass.hpp
  • eb5c097: 8262389: Use permitted_enctypes if default_tkt_enctypes or default_tgs_enctypes is not present
  • bfb034a: 8264524: jdk/internal/platform/docker/TestDockerMemoryMetrics.java fails due to swapping not working
  • ... and 10 more: https://git.openjdk.java.net/jdk/compare/ff223530b66a83fb77c477da97a2dd60df448ec8...master

Your commit was automatically rebased without conflicts.

Pushed as commit 774e5ae.

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

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