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

[Stefan Karlsson]

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
>>