RFR: 8230661: ZGC: Stop reloading oops in load barriers

[erik.osterlund at oracle.com]

Hi Stefan,

Thanks for the review. I will file an RFE about the proposed 
enhancements (which I agree about).

/Erik

On 11/12/19 4:05 PM, Stefan Karlsson wrote:
> Hi,
>
> This looks good to me.
>
> As a separate patch, I think that we should:
>
> 1) swap places between the calls to ZResurrection::unblock and 
> _unload.purge(). There every stale weak/phantom reference has been 
> cleaned out at this point, and there's no need to hold the 
> resurrection blocked windows open longer than necessary.
>
> 2) change the implementation of ZResurrection to use 
> Atomic::load/store now that we rely on the handshakes to prevent 
> someone from having a weak oop to a "dead" object while finding out 
> that the resurrection has been unblocked.
>
> Thanks,
> StefanK
>
> On 2019-11-11 21:41, Per Liden wrote:
>> Erik, Stefan and I discussed the original patch and agreed to make 
>> some adjustments. The changes are:
>>
>> 1) Broke out self healing logic into ZBarrier::self_heal(), and 
>> restructured ZBarrier::barrier() and ZBarrier::weak_barrier() to use 
>> that function.
>>
>> 2) Made sure the slow path is only ever executed once, even when self 
>> healing is re-applied.
>>
>> 3) In the resurrection blocked window, allow weak oop refs to be 
>> healed to the good state, not just the remapped state.
>>
>> 4) Moved the handshake in ZUnload::unload() out into ZHeap, to make 
>> it more front-and-center, since it's no longer just needed for class 
>> unloading.
>>
>> Diff: http://cr.openjdk.java.net/~pliden/8230661/webrev.1-diff
>> Full: http://cr.openjdk.java.net/~pliden/8230661/webrev.1
>> Testing: Passed tier 1-7
>>
>> With these adjustments this patch looks good to me!
>>
>> cheers,
>> Per
>>
>> On 10/28/19 5:44 PM, Erik Österlund wrote:
>>> Oops. CR link was a bug link. For anyone that couldn't figure out 
>>> what the CR link could possibly be, here it is:
>>> http://cr.openjdk.java.net/~eosterlund/8230661/webrev.00/
>>>
>>> /Erik
>>>
>>> On 2019-10-28 17:38, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> In ZGC, an oop is first loaded somewhere, by e.g. JIT compiled 
>>>> code. Then it passes a load barrier that typically does not take a 
>>>> slow path. But when it does take a slow path, the oop is sometimes 
>>>> reloaded, at historically three different places, and now two places.
>>>>
>>>> 1) We used to do that as part of the mechanism that transferred 
>>>> execution to the slow path because it was easier to write that stub 
>>>> code if the original oop died. Since then, the compiler slow paths 
>>>> have been rewritten to not reload the oop.
>>>>
>>>> 2) Once in the slow path, we sometimes reload weak oops during the 
>>>> resurrection block window, because there used to be a race when it 
>>>> closed. After concurrent class unloading integrated, there is a 
>>>> thread-local handshake before closing the resurrection block 
>>>> window. Therefore, that race no longer exists (when class unloading 
>>>> is used).
>>>>
>>>> 3) Once the final oop of a slow path has been determined, 
>>>> self-healing kicks in. The self-healing CAS may fail. When it does, 
>>>> the oop is reloaded. But this is completely unnecessary.
>>>>
>>>> With obstacle 1 gone, and 2 and 3 having no reason to be in the 
>>>> code any more, I propose to get rid of all reloading of the oops in 
>>>> the slow paths, so that it becomes easier to reason about the code. 
>>>> The object captured by the original load, is then always the same 
>>>> object as the object found after the load barrier completes, 
>>>> although possibly with a new bit representation.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8230661
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8230661
>>>>
>>>> Thanks,
>>>> /Erik
>>>