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

[Vladimir Kozlov]

Yes, this looks good to me too.

Thanks,
Vladimir

On 3/6/19 3:14 AM, 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
>