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