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

[Richard Reingruber]

On Mon, 14 Sep 2020 05:02:42 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Changes based on dholmes' feedback.
>>   
>>   EscapeBarrier::sync_and_suspend_all():
>>   
>>     - Set suspend flags before handshake because then the setting will be visible
>>       after leaving the _thread_blocked state in JavaThread::wait_for_object_deoptimization()
>>   
>>   JavaThread::wait_for_object_deoptimization()
>>   
>>     - Reuse SpinYield instead of new custom spinning code.
>>   
>>     - Do safepoint check after the loop. This is possible because the
>>       set_obj_deopt_flag is done before the handshake.
>>   
>>     - Don't set_suspend_equivalent() anymore for more simplicity. It's just an
>>       optimization (that won't pay off here).
>>   
>>   Added/updated source code comments.
>>   
>>   Additional smaller enhancements.
>
> 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.

Done.

> 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.

Fixed.

> 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.

For now I reshaped and hopefully also simplified the method. It should resemble
its model java_suspend_self_with_safepoint_check() very much now. With this
similarity I'd see the added complexity to be small. In fact
wait_for_object_deoptimization() could be a drop-in replacement for
java_suspend_self_with_safepoint_check(). Well, the condition in the callers
would have to be adapted.

> 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.

I added a comment explaining it before the method definition.

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

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