RFR: 8198286: Direct memory accessors in typeArrayOop.hpp should use Access API

[Erik Österlund]

Hi Roman,

On 2018-02-19 18:24, Roman Kennke wrote:
> Hi Erik,
>
> thanks for proposing this. I only eyeballed it and base my comments on
> what I've seen. See inline:
>
>> I see there is a need to resolve a stable address for some objects to bulk
>> access primitives. The code base is full of assumptions that no barriers are
>> needed for such address resolution. It looks like the proposed approach is
>> to one by one hunt down all such callsites. I could find some places where
>> such barriers are missing.
> I see. This is because you probably grepped for '_addr' and found all
> the raw field accessors too :-) I meant to address them separately,
> but I am not opposed to also handle them here, as it's more or less
> the same issue.

Okay, then I will handle that here as well.

> Also, you probably replied to the wrong thread ;-)

Yeah, sorry about that.

>
>> To make the code as maintainable as possible, I would like to propose a
>> slightly different take on this, and would love to hear if this works for
>> Shenandoah or not. The main idea is to annotate places where we do *not*
>> want GC address resolution for internal pointers to objects, instead of
>> where we want it, as it seems to be the common case that we do want to
>> resolve the address.
>>
>> In some more detail:
>>
>> 1) Rip out the *_addr fascilities not used (a whole bunch on oopDesc).
>> 2) Ignore the difference between read/write resolution (write resolution
>> handles both reads and writes). Instead introduce an oop
>> resolve_stable_addr(oop) function in Access. This makes it easier to use.
>> 3) Identify as few callsites as possible for this function. I'm thinking
>> arrayOop::base() and a few strange exceptions.
>> 4) Identify the few places where we explicitly do *not* want address
>> resolution, like calls from GC, and replace them with *_addr_raw variants.
>> 5) Have a switch in barrierSetConfig.hpp that determines whether the build
>> needs to support not to-space invariant GCs or not.
>
>> With these changes, the number of callsites have been kept down to what I
>> believe to be a minimum. And yet it covers some callsites that you
>> accidentally missed (e.g. jvmciCodeInstaller.cpp). Existing uses of the
>> various *_addr fascilities can in most cases continue to do what they have
>> done in the past. And new uses will not be surprised that they accidentally
>> missed some barriers. It will be solved automagically.
> Ok, this seems like a reasonable approach.

I'm glad you think so!

> The only thing that I am worried is that a theoritical future GC might
> want to handle reads totally different than writes, and the actions
> taken for reads is not a subset of what it needs to do for writes, but
> I said 'theoretical' because I cannot think of a situation where this
> might be neccesary. It's still only the perfectionist in me that wants
> a distinction between reads and writes. Also, as you say, if you hand
> back a pointer to some memory, it's actually safer to enforce the
> stronger write invariant to prevent accidental misuse. We might
> actually figure out a better way to deal with many of the uses of
> _addr(), for example, as Kim suggested, make the bulk memory transfers
> use the arraycopy (or similar) API to begin with.

Any hypothetical future GC that does not support handing out addresses 
to typeArrayOop where it is uncertain whether loads or stores will be 
performed, will have a lot of trouble supporting, e.g. JNI get critical 
and similar functionality. Therefore I think it is safe to say that it 
is unlikely to ever happen. And if it does, then I think we can just 
deal with that then.

>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/typearray_resolve/webrev.00/
>  From the looks of it, it should work.

Excellent.

> One thing that's always worries me is the use of _raw() methods. What
> does that mean? No GC barriers needed at all? How does the caller
> determine if it's safe to use _raw() over the usual accessor? I have
> seen uses of _raw() in the past that did require barriers in
> Shenandoah...
> as far as I can tell, those raw accessors are only safe to use by the
> GCs themselves.

For me _raw() means no GC barriers used (as opposed to needed), and yes 
it is intended to be used only by GC. I will soon upload a webrev that 
enforces this.

> Other than that, I believe the approach should work.

Excellent.

> Do you want to take over the issue?
> https://bugs.openjdk.java.net/browse/JDK-8198286

Sure. :)

Thanks,
/Erik

>
> Thanks for helping with this!
> /Roman