RFR: 8264742: member variable _monitor of MonitorLocker is redundant

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8264742: member variable _monitor of MonitorLocker is redundant

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

-------------

Commit messages:
 - member variable _monitor of MonitorLocker is redundant

Changes: https://git.openjdk.java.net/jdk/pull/3350/files
 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
  Patch: https://git.openjdk.java.net/jdk/pull/3350.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3350/head:pull/3350

PR: https://git.openjdk.java.net/jdk/pull/3350
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264742: member variable _monitor of MonitorLocker is redundant

David Holmes
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
>
> Changes: https://git.openjdk.java.net/jdk/pull/3350/files
>   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
>    Patch: https://git.openjdk.java.net/jdk/pull/3350.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/3350/head:pull/3350
>
> PR: https://git.openjdk.java.net/jdk/pull/3350
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264742: member variable _monitor of MonitorLocker is redundant

Liu, Xin
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" <[hidden email]> 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
    >
    > Changes: https://git.openjdk.java.net/jdk/pull/3350/files
    >   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
    >    Patch: https://git.openjdk.java.net/jdk/pull/3350.diff
    >    Fetch: git fetch https://git.openjdk.java.net/jdk pull/3350/head:pull/3350
    >
    > PR: https://git.openjdk.java.net/jdk/pull/3350
    >

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264742: member variable _monitor of MonitorLocker is redundant

Coleen Phillimore-3
In reply to this post by Xin Liu
On Tue, 6 Apr 2021 04:05:21 GMT, Xin Liu <[hidden email]> wrote:

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

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

-------------

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3350
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264742: member variable _monitor of MonitorLocker is redundant

Xin Liu
On Tue, 6 Apr 2021 12:18:41 GMT, Coleen Phillimore <[hidden email]> wrote:

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/3350
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264742: member variable _monitor of MonitorLocker is redundant

David Holmes-2
In reply to this post by Xin Liu
On Tue, 6 Apr 2021 04:05:21 GMT, Xin Liu <[hidden email]> wrote:

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

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

Changes look good.

Thanks,
David

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3350
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264742: member variable _monitor of MonitorLocker is redundant

Paul Hohensee-4
In reply to this post by Xin Liu
On Tue, 6 Apr 2021 04:05:21 GMT, Xin Liu <[hidden email]> wrote:

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

Marked as reviewed by phh (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3350
Reply | Threaded
Open this post in threaded view
|

Integrated: 8264742: member variable _monitor of MonitorLocker is redundant

Xin Liu
In reply to this post by Xin Liu
On Tue, 6 Apr 2021 04:05:21 GMT, Xin Liu <[hidden email]> wrote:

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

This pull request has now been integrated.

Changeset: 774e5ae0
Author:    Xin Liu <[hidden email]>
Committer: Paul Hohensee <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/774e5ae0
Stats:     15 lines in 1 file changed: 5 ins; 1 del; 9 mod

8264742: member variable _monitor of MonitorLocker is redundant

Reviewed-by: coleenp, dholmes, phh

-------------

PR: https://git.openjdk.java.net/jdk/pull/3350