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

[Erik Österlund]

Hi Roman,

On 2018-07-26 13:09, 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.

>> In oops/oop.cpp:
>> Rather than changing the metadata accessors to always assume raw
>> accesses are enough for all metadata accesses, I would prefer to have
>> explicit metadata_field_raw accessors instead where we expect this (even
>> if that turns out to be always).
> Alright, I unwinded it and came up with a minimal use of
> metadata_field_raw(). It requires splicing Class:as_Klass() into a
> _raw() variant to be used from oop iterator paths. But it's not as bad
> as I thought.
>
> Updated patch:
> http://cr.openjdk.java.net/~rkennke/JDK-8206457/webrev.01/
>
> Do you like it better? Can you check the compile-error that I mentioned
> above?

Looks better now, yeah.

Thanks,
/Erik

> Thanks, Roman
>
>
>> Thanks,
>> /Erik
>>
>> On 2018-07-06 16:46, Roman Kennke wrote:
>>>    We have several code paths going out from oop_iterate() methods that
>>> lead to GC barriers. This is not only inefficient but outright wrong.
>>> oop_iterate() is normally used by GC and GC need to see the raw stuff,
>>> not some resolved objects. In Shenandoah's full-GC it's fatal to attempt
>>> to read objects's forwarding pointers, because it's temporarily pointing
>>> to nowhere land.
>>>
>>> I propose to selectively use _raw() variants of the various accessors
>>> that are used on oop_iterate() paths. This means to introduce an
>>> oopDesc::int_field_raw(). I also propose to change metadata_field()
>>> accessors to always use raw access wholesale. This is only used to load
>>> the Klass* field, which is immutable and thus doesn't require barriers.
>>>
>>> The log_* statements in instanceRefKlass.inline.hpp surely don't need
>>> barriers. I turned them into raw accessors as well.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8206457?filter=-1
>>> Webrev:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8206457/webrev.00/
>>>
>>> Test: passes hotspot-tier1 here.
>>>
>>> Can I please get review?
>>>
>>> Roman
>>>
>