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

[Roman Kennke]

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.

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

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

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.

> Webrev:
> http://cr.openjdk.java.net/~eosterlund/typearray_resolve/webrev.00/

>From the looks of it, it should work.

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.

Other than that, I believe the approach should work.

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


Thanks for helping with this!
/Roman