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

Reply via email to