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

[Serguei Spitsyn]

On Tue, 22 Sep 2020 00:38:43 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Reviewed in the good old hg times :)
>> See also http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041453.html
>
> Hi Richard,
> 
> Could you consider to place the classes EscapeBarrier and JvmtiDeferredUpdates into theyr own .hpp/.cpp files? The
> class JvmtiDeferredUpdates would be better to put into the folder 'prims' then.
> src/hotspot/share/opto/macro.cpp:
> @@ -1091,11 +1091,11 @@
>  bool PhaseMacroExpand::eliminate_allocate_node(AllocateNode *alloc) {
>    // Don't do scalar replacement if the frame can be popped by JVMTI:
>    // if reallocation fails during deoptimization we'll pop all
>    // interpreter frames for this compiled frame and that won't play
>    // nice with JVMTI popframe.
> -  if (!EliminateAllocations || JvmtiExport::can_pop_frame() || !alloc->_is_non_escaping) {
> +  if (!EliminateAllocations || !alloc->_is_non_escaping) {
>      return false;
>    }
>   
> I wonder if the comment is still correct after you removed the check for JvmtiExport::can_pop_frame().
> 
> src/hotspot/share/runtime/deoptimization.hpp:
> 
> +  EscapeBarrier(JavaThread* calling_thread, JavaThread* deoptee_thread, bool barrier_active)
> +    : _calling_thread(calling_thread), _deoptee_thread(deoptee_thread),
> +      _barrier_active(barrier_active && (JVMCI_ONLY(UseJVMCICompiler) NOT_JVMCI(false)
> +                      COMPILER2_PRESENT(|| DoEscapeAnalysis)))
> . . . . . . . . .
> +
> +  // Revert ea based optimizations for all java threads
> +  EscapeBarrier(JavaThread* calling_thread, bool barrier_active)
> +    : _calling_thread(calling_thread), _deoptee_thread(NULL),
> 
> Nit: would better to make the parameter deoptee_thread to be the 3rd to better mach the seconf constructor.
> 
> +  bool all_threads()    const { return _deoptee_thread == NULL; }            // Should revert optimizations for all
> threads. +  bool self_deopt()     const { return _calling_thread == _deoptee_thread; } // Current thread deoptimizes
> its own objects. +  bool barrier_active() const { return _barrier_active; }                    // Inactive barriers are
> created if no local objects can escape.
> I'd suggest to put comments in a line before function definitions as it is done for other declarations/definitions.

src/hotspot/share/runtime/deoptimization.cpp:
 @@ -349,12 +408,12 @@
 
   // Now that the vframeArray has been created if we have any deferred local writes
   // added by jvmti then we can free up that structure as the data is now in the
   // vframeArray
 
-  if (thread->deferred_locals() != NULL) {
-    GrowableArray<jvmtiDeferredLocalVariableSet*>* list = thread->deferred_locals();
+  if (JvmtiDeferredUpdates::deferred_locals(thread) != NULL) {
+    GrowableArray<jvmtiDeferredLocalVariableSet*>* list = JvmtiDeferredUpdates::deferred_locals(thread);
     int i = 0;
     do {
       // Because of inlining we could have multiple vframes for a single frame
       // and several of the vframes could have deferred writes. Find them all.
       if (list->at(i)->id() == array->original().id()) {

@@ -365,13 +424,14 @@
       } else {
         i++;
       }
     } while ( i < list->length() );
     if (list->length() == 0) {
-      thread->set_deferred_locals(NULL);
-      // free the list and elements back to C heap.
-      delete list;
+      JvmtiDeferredUpdates* updates = thread->deferred_updates();
+      thread->set_deferred_updates(NULL);
+      // free deferred updates.
+      delete updates;
     }
It is not clear why the 'list' is not deleted anymore. If it is intentional then could you, please, add a comment with
an explanation?

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

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