RFR: JDK-8206457: Code paths from oop_iterate() must use barrier-free access

[Roman Kennke]

Erik: thanks for reviewing!

Anybody else wants to review this?

Thanks, Roman


> Yes, looks good.
> 
> Thanks,
> /Erik
> 
> On 2018-08-09 10:10, Roman Kennke wrote:
>>
>>>>>>>> Hi Erik,
>>>>>>>> thanks for reviewing!
>>>>>>>>
>>>>>>>>> In instanceRefKlass.inline.hpp:
>>>>>>>>>
>>>>>>>>> Are these changes to logging required for Shenandoah not to
>>>>>>>>> crash? It
>>>>>>>>> appears to me that for ZGC, it would print the wrong addresses if
>>>>>>>>> barriers were not used. That's why I wonder if this is a strict
>>>>>>>>> requirement or not for Shenandoah to work.
>>>>>>>> We do need to avoid read-barriers on those paths. The problem is
>>>>>>>> that
>>>>>>>> during full-GC we temporarily don't have the fwd-ptr available
>>>>>>>> (it's a
>>>>>>>> sliding mark-compact algorithm). However, we can work around
>>>>>>>> this by
>>>>>>>> not
>>>>>>>> using the base+offset variants like in the patch. However, this
>>>>>>>> seems to
>>>>>>>> make the Access API unhappy at compile-time when using
>>>>>>>> ON_UNKNOWN_OOP_REF. Can you check this? I've no clue where to look.
>>>>>>> The reason is that wherever ON_UNKNOWN_OOP_REF is used, the backend
>>>>>>> needs to be able to determine the exact strength. And to do that,
>>>>>>> the
>>>>>>> backend needs to be able to determine of this is a referent
>>>>>>> field. And
>>>>>>> to do that, it needs a base pointer.
>>>>>>>
>>>>>>> I'm not 100% sure what I think is a good solution to this. I
>>>>>>> wonder if
>>>>>>> along the lines of introducing these resolve for read/write
>>>>>>> decorators
>>>>>>> (which it looks like we will be needing anyway), there could be a
>>>>>>> do not
>>>>>>> resolve decorator that could be passed in to determining how to
>>>>>>> resolve
>>>>>>> the access. Default for stores could be ACCESS_WRITE, for loads
>>>>>>> ACCESS_READ, for atomics ACCESS_READ | ACCESS_WRITE, and explicitly
>>>>>>> setting ACCESS_NONE meaning don't resolve this one. Maybe the prefix
>>>>>>> ought to be RESOLVE_READ / RESOLVE_WRITE / RESOLVE_NONE instead
>>>>>>> though
>>>>>>> to be more specific.
>>>>>> We are in instanceRefKlass, and we should be able to determine the
>>>>>> reference strength statically, and pass in the correct ON_XXX_OOP_REF
>>>>>> decorator, right? E.g. via InstanceKlass::reference_type() ? Or would
>>>>>> that not work?
>>>>> That should probably do the trick, yes.
>>>> not 100% sure this is the correct ReferenceType -> decorators mapping?
>>>>
>>>> Incremental:
>>>> http://cr.openjdk.java.net/~rkennke/JDK-8206457/webrev.02.diff/
>>>> Full patch:
>>>> http://cr.openjdk.java.net/~rkennke/JDK-8206457/webrev.02/
>>>>
>>> Ping?
>>
>> Erik, is this good now?
>>
>> Thanks, Roman
>