RFR (S): 8003135: HotSpot inlines and hoists the Thread.currentThread().isInterrupted() out of the loop
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Thu Dec 13 16:29:49 PST 2012
John,
Thanks for the review!
>> Proposed fix is cleaner that [1] (no explicit memory state management).
>> The place for the barrier is deliberately chosen earlier than
necessary and it blocks hoisting of some loop invariants when
Thread.isInterrupted() is called in a loop. But I think it's a fair
price since I don't think that Thread.isInterrupted(Z)Z performance in
tight loops is critical.
>
> I don't see any new barrier in the simple fix. Where is it? It
looks like the load of thr._interrupted can be hoisted under the same
circumstances as before. If the slow path is somehow turned into a loop
exit (via uncommon trap, maybe), then the memory state will fold up and
the bit load can be hoisted.
I placed it at the very beginning of the function.
I'm not very comfortable with memory state management so far, but I
don't see how it differs from "fast" fix. Memory graph seems almost the
same between these 2 versions. The only difference is that the check of
current thread is after the barrier.
I'll experiment with converting slow call into uncommon trap and see how
it works.
> It seems like your "fast" fix is more robust, for this reason.
>
> Also, it's not your bug, but I'm not convinced that "slow_val =
intcon(1)" is correct. I think another thread could come in and clear
the interrupted bit, and this call to the native function could return
false.
Good catch! You are right, there's a window (though it's very small)
when another thread can interrupt the thread. I didn't like
known_current_thread variable and now I have a reason to completely
remove it :-)
Best regards,
Vladimir Ivanov
>> Proposed fix is cleaner that [1] (no explicit memory state management).
>> The place for the barrier is deliberately chosen earlier than necessary and it blocks hoisting of some loop invariants when Thread.isInterrupted() is called in a loop. But I think it's a fair price since I don't think that Thread.isInterrupted(Z)Z performance in tight loops is critical.
>
> I don't see any new barrier in the simple fix. Where is it? It looks like the load of thr._interrupted can be hoisted under the same circumstances as before. If the slow path is somehow turned into a loop exit (via uncommon trap, maybe), then the memory state will fold up and the bit load can be hoisted.
I placed it at the beginning of the function.
> It seems like your "fast" fix is more robust, for this reason.
>
> Also, it's not your bug, but I'm not convinced that "slow_val = intcon(1)" is correct. I think another thread could come in and clear the interrupted bit, and this call to the native function could return false.
Good catch! You are right, there's a very small window there.
>> If you prefer [1] let me know.
>>
>> Also, cleaned up the code around a little.
>
> Good cleanups.
>
> — John
>
>> Testing: failing test, manual (verified generated code), JPRT (in progress)
>>
>> Thanks!
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> [1] http://cr.openjdk.java.net/~vlivanov/8003135/fast/webrev.00
>
More information about the hotspot-compiler-dev
mailing list