On Mon, 14 Sep 2020 05:02:42 GMT, David Holmes <dhol...@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
