RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents

[David Holmes]

On Thu, 10 Sep 2020 20:48:23 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

> Hi,
> 
> this is the continuation of the review of the implementation for:
> 
> https://bugs.openjdk.java.net/browse/JDK-8227745
> https://bugs.openjdk.java.net/browse/JDK-8233915
> 
> It allows for JIT optimizations based on escape analysis even if JVMTI agents acquire capabilities to access references
> to objects that are subject to such optimizations, e.g. scalar replacement. The implementation reverts such
> optimizations just before access very much as when switching from JIT compiled execution to the interpreter, aka
> "deoptimization".  Webrev.8 was the last one before before the transition to Git/Github:
> 
> http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/
> 
> Thanks, Richard.

I can't review this in-depth as I am not familiar with too much of the code. I have looked at some of the key threading
related parts and have made a number of comments.

src/hotspot/share/compiler/compileBroker.cpp line 831:

> 829:       MonitorLocker ml(dt, EscapeBarrier_lock, Mutex::_no_safepoint_check_flag);
> 830:       static int single_thread_count = 0;
> 831:       enter_single_loop = single_thread_count++ < DeoptimizeObjectsALotThreadCountSingle;

The update to `single_thread_count` is not atomic.

src/hotspot/share/compiler/compileBroker.cpp line 844:

> 842: void DeoptimizeObjectsALotThread::deoptimize_objects_alot_loop_single() {
> 843:   HandleMark hm(this);
> 844:   while (!this->is_terminated()) {

The terminated state of a thread is used to communicate to other threads whether this thread has terminated. It isn't
something that can be used to control the work-loop of the thread itself! This is an infinite loop as far as I can see.

src/hotspot/share/compiler/compileBroker.cpp line 862:

> 860: void DeoptimizeObjectsALotThread::deoptimize_objects_alot_loop_all() {
> 861:   HandleMark hm(this);
> 862:   while (!is_terminated()) {

Same comment as above.

src/hotspot/share/opto/macro.cpp line 1096:

> 1094:   // interpreter frames for this compiled frame and that won't play
> 1095:   // nice with JVMTI popframe.
> 1096:   if (!EliminateAllocations || !alloc->_is_non_escaping) {

The comment needs to be updated to match the new code.

src/hotspot/share/prims/whitebox.cpp line 884:

> 882:
> 883: WB_ENTRY(jboolean, WB_IsFrameDeoptimized(JNIEnv* env, jobject o, jint depth))
> 884:   JavaThread* t = JavaThread::current();

A WB_ENTRY is a JNI_ENTRY which means the current JavaThread is already available via the `thread` variable.

src/hotspot/share/runtime/thread.cpp line 2647:

> 2645:   bool should_spin_wait = true;
> 2646:   do {
> 2647:     set_thread_state(_thread_blocked);

It isn't obvious that this raw thread state change is safe.

src/hotspot/share/runtime/thread.cpp line 2660:

> 2658:     // showed 5% better performance when spinning.
> 2659:     if (should_spin_wait) {
> 2660:       // Inspired by HandshakeSpinYield

Can we not reuse any existing spinning code in the VM?

src/hotspot/share/runtime/thread.cpp line 2640:

> 2638: }
> 2639:
> 2640: void JavaThread::wait_for_object_deoptimization() {

I find this method very complex, and the fact it needs to recheck many of the conditions already checked in the calling
code, suggests to me that the structure here is not quite right. Perhaps
JavaThread::handle_special_runtime_exit_condition should be the place where we loop over each of the potential
conditions, instead of calling per-condition code that then loops and potentially rechecks other conditions.

src/hotspot/share/runtime/thread.cpp line 1926:

> 1924:       delete dlv;
> 1925:     } while (deferred->length() != 0);
> 1926:     delete deferred_updates();

This looks suspect - we delete what is pointed to but we don't NULL the field.

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

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