On Tue, 22 Sep 2020 00:38:43 GMT, Serguei Spitsyn <sspit...@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
