[13] RFR(M): 8163511: Allocation of compile task fails with assert: "Leaking compilation tasks?"

[Tobias Hartmann]

Hi Erik,

thanks again for the review.

Best regards,
Tobias

On 06.03.19 12:14, Erik Österlund wrote:
> Hi Tobias,
> 
> On 2019-03-06 11:50, Tobias Hartmann wrote:
>> Hi Erik,
>>
>> On 05.03.19 09:09, Erik Osterlund wrote:
>>> Liveness of objects only change in safepoints. So if you are in the vm thread state, and ask if a
>>> holder is alive, its result is stable.
>>>
>>> With concurrent class unloading, marking terminates in a safepoint. At that snapshot in time, it
>>> is determined what is alive (strongly and through finalizers) and dead (although it might be
>>> lazily evaluated during concurrent execution). When that safepoint is released, that view will be
>>> consistent.
>>>
>>> As there are seemingly no safepoint checks in this code, all liveness information will be
>>> consistent and none of the imagined races exist with concurrent class unloading. It behaves as if
>>> it was all done in a safepoint, even though it was not.
>>>
>>> The key is that the klass_holder liveness (as well as the jweaks you use) is determined using a
>>> phantom load via the access API. That phantom load gives consistent results in the vm thread
>>> state as-if reference processing was done in a safepoint. The phantom loads can lazily compute
>>> what the value would have been, had reference processing been done in a safepoint, as it has
>>> stable liveness information from that safepoint. Then concurrent reference processing races for
>>> producing the same values.
>>>
>>> Basically there is nothing to worry about here as far as I can see. Looks good.
>> Thanks a lot for the thorough explanation and the review.
>>
>>> Although freeing the jweaks in one place instead of two would make the code easier to follow. Up
>>> to you though and I don’t need another webrev for that.
>> I could only factor the freeing into a separate method and call it from CompileTask::free() and
>> CompileTask::select_for_compilation(). But I don't think that would make the code easier to read.
> 
> Ah, okay.
> 
>> Relying on the assumption that either both _method_holder and _hot_method_holder are weak references
>> or both are strong, I've slightly refactored the code in CompileTask::free():
>>
>> http://cr.openjdk.java.net/~thartmann/8163511/webrev.02
> 
> The new webrev looks good to me.
> 
> Thanks,
> /Erik
> 
>>
>> Best regards,
>> Tobias
>