Hi David,
> Some further queries/concerns:
>
> src/hotspot/share/runtime/objectMonitor.cpp
>
> Can you please explain the changes to ObjectMonitor::wait:
>
> ! _recursions = save // restore the old recursion count
> ! + jt->get_and_reset_relock_count_after_wait(); //
> increased by the deferred relock count
>
> what is the "deferred relock count"? I gather it relates to
>
> "The code was extended to be able to deoptimize objects of a frame that
> is not the top frame and to let another thread than the owning thread do
> it."
Yes, these relate. Currently EA based optimizations are reverted, when a
compiled frame is replaced
with corresponding interpreter frames. Part of this is relocking objects with
eliminated
locking. New with the enhancement is that we do this also just before object
references are acquired
through JVMTI. In this case we deoptimize also the owning compiled frame C and
we register
deoptimized objects as deferred updates. When control returns to C it gets
deoptimized, we notice
that objects are already deoptimized (reallocated and relocked), so we don't do
it again (relocking
twice would be incorrect of course). Deferred updates are copied into the new
interpreter frames.
Problem: relocking is not possible if the target thread T is waiting on the
monitor that needs to be
relocked. This happens only with non-local objects with EliminateNestedLocks.
Instead relocking is
deferred until T owns the monitor again. This is what the piece of code above
does.
> which I don't like the sound of at all when it comes to ObjectMonitor
> state. So I'd like to understand in detail exactly what is going on here
> and why. This is a very intrusive change that seems to badly break
> encapsulation and impacts future changes to ObjectMonitor that are under
> investigation.
I would not regard this as breaking encapsulation. Certainly not badly.
I've added a property relock_count_after_wait to JavaThread. The property is
well
encapsulated. Future ObjectMonitor implementations have to deal with recursion
too. They are free in
choosing a way to do that as long as that property is taken into account. This
is hardly a
limitation.
Note also that the property is a straight forward extension of the existing
concept of deferred
local updates. It is embedded into the structure holding them. So not even the
footprint of a
JavaThread is enlarged if no deferred updates are generated.
> ---
>
> src/hotspot/share/runtime/thread.cpp
>
> Can you please explain why JavaThread::wait_for_object_deoptimization
> has to be handcrafted in this way rather than using proper transitions.
>
I wrote wait_for_object_deoptimization taking
JavaThread::java_suspend_self_with_safepoint_check
as template. So in short: for the same reasons :)
Threads reach both methods as part of thread state transitions, therefore
special handling is
required to change thread state on top of ongoing transitions.
> We got rid of "deopt suspend" some time ago and it is disturbing to see
> it being added back (effectively). This seems like it may be something
> that handshakes could be used for.
Deopt suspend used to be something rather different with a similar name[1]. It
is not being added back.
I'm actually duplicating the existing external suspend mechanism, because a
thread can be suspended
at most once. And hey, and don't like that either! But it seems not unlikely
that the duplicate can
be removed together with the original and the new type of handshakes that will
be used for
thread suspend can be used for object deoptimization too. See today's
discussion in JDK-8227745 [2].
Thanks, Richard.
[1] Deopt suspend was something like an async. handshake for architectures with
register windows,
where patching the return pc for deoptimization of a compiled frame was
racy if the owner thread
was in native code. Instead a "deopt" suspend flag was set on which the
thread patched its own
frame upon return from native. So no thread was suspended. It got its name
only from the name of
the flags.
[2] Discussion about using handshakes to sync. with the target thread:
https://bugs.openjdk.java.net/browse/JDK-8227745?focusedCommentId=14306727&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14306727
-----Original Message-----
From: David Holmes <david.hol...@oracle.com>
Sent: Freitag, 13. Dezember 2019 00:56
To: Reingruber, Richard <richard.reingru...@sap.com>;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in
the Presence of JVMTI Agents
Hi Richard,
Some further queries/concerns:
src/hotspot/share/runtime/objectMonitor.cpp
Can you please explain the changes to ObjectMonitor::wait:
! _recursions = save // restore the old recursion count
! + jt->get_and_reset_relock_count_after_wait(); //
increased by the deferred relock count
what is the "deferred relock count"? I gather it relates to
"The code was extended to be able to deoptimize objects of a frame that
is not the top frame and to let another thread than the owning thread do
it."
which I don't like the sound of at all when it comes to ObjectMonitor
state. So I'd like to understand in detail exactly what is going on here
and why. This is a very intrusive change that seems to badly break
encapsulation and impacts future changes to ObjectMonitor that are under
investigation.
---
src/hotspot/share/runtime/thread.cpp
Can you please explain why JavaThread::wait_for_object_deoptimization
has to be handcrafted in this way rather than using proper transitions.
We got rid of "deopt suspend" some time ago and it is disturbing to see
it being added back (effectively). This seems like it may be something
that handshakes could be used for.
Thanks,
David
-----
On 12/12/2019 7:02 am, David Holmes wrote:
> On 12/12/2019 1:07 am, Reingruber, Richard wrote:
>> Hi David,
>>
>> > Most of the details here are in areas I can comment on in detail,
>> but I
>> > did take an initial general look at things.
>>
>> Thanks for taking the time!
>
> Apologies the above should read:
>
> "Most of the details here are in areas I *can't* comment on in detail ..."
>
> David
>
>> > The only thing that jumped out at me is that I think the
>> > DeoptimizeObjectsALotThread should be a hidden thread.
>> >
>> > + bool is_hidden_from_external_view() const { return true; }
>>
>> Yes, it should. Will add the method like above.
>>
>> > Also I don't see any testing of the DeoptimizeObjectsALotThread.
>> Without
>> > active testing this will just bit-rot.
>>
>> DeoptimizeObjectsALot is meant for stress testing with a larger
>> workload. I will add a minimal test
>> to keep it fresh.
>>
>> > Also on the tests I don't understand your @requires clause:
>> >
>> > @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled &
>> > (vm.opt.TieredCompilation != true))
>> >
>> > This seems to require that TieredCompilation is disabled, but
>> tiered is
>> > our normal mode of operation. ??
>> >
>>
>> I removed the clause. I guess I wanted to target the tests towards the
>> code they are supposed to
>> test, and it's easier to analyze failures w/o tiered compilation and
>> with just one compiler thread.
>>
>> Additionally I will make use of
>> compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests.
>>
>> Thanks,
>> Richard.
>>
>> -----Original Message-----
>> From: David Holmes <david.hol...@oracle.com>
>> Sent: Mittwoch, 11. Dezember 2019 08:03
>> To: Reingruber, Richard <richard.reingru...@sap.com>;
>> serviceability-dev@openjdk.java.net;
>> hotspot-compiler-...@openjdk.java.net;
>> hotspot-runtime-...@openjdk.java.net
>> Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better
>> Performance in the Presence of JVMTI Agents
>>
>> Hi Richard,
>>
>> On 11/12/2019 7:45 am, Reingruber, Richard wrote:
>>> Hi,
>>>
>>> I would like to get reviews please for
>>>
>>> http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/
>>>
>>> Corresponding RFE:
>>> https://bugs.openjdk.java.net/browse/JDK-8227745
>>>
>>> Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915
>>> And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1]
>>>
>>> Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without
>>> issues (thanks!). In addition the
>>> change is being tested at SAP since I posted the first RFR some
>>> months ago.
>>>
>>> The intention of this enhancement is to benefit performance wise from
>>> escape analysis even if JVMTI
>>> agents request capabilities that allow them to access local variable
>>> values. E.g. if you start-up
>>> with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then
>>> escape analysis is disabled right
>>> from the beginning, well before a debugger attaches -- if ever one
>>> should do so. With the
>>> enhancement, escape analysis will remain enabled until and after a
>>> debugger attaches. EA based
>>> optimizations are reverted just before an agent acquires the
>>> reference to an object. In the JBS item
>>> you'll find more details.
>>
>> Most of the details here are in areas I can comment on in detail, but I
>> did take an initial general look at things.
>>
>> The only thing that jumped out at me is that I think the
>> DeoptimizeObjectsALotThread should be a hidden thread.
>>
>> + bool is_hidden_from_external_view() const { return true; }
>>
>> Also I don't see any testing of the DeoptimizeObjectsALotThread. Without
>> active testing this will just bit-rot.
>>
>> Also on the tests I don't understand your @requires clause:
>>
>> @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled &
>> (vm.opt.TieredCompilation != true))
>>
>> This seems to require that TieredCompilation is disabled, but tiered is
>> our normal mode of operation. ??
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Richard.
>>>
>>> [1] Experimental fix for JDK-8214584 based on JDK-8227745
>>>
>>> http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch
>>>
>>>
