RFR: JDK-8222281: GC interface for load-klass: runtime part

[Per Liden]

Hi Roman,

On 04/12/2019 05:13 PM, Roman Kennke wrote:
> Alright, I am experimenting with this. It turns out that there are a
> *lot* of paths that call into oopDesc::klass() which need extra
> handling, all of which come out of the GC. Example is oopDesc::size()
> and oopDesc::oop_iterate() and various others.

It's till not quite clear to me why you need this. Put another way, I 
think you should really try to design this such that you don't need 
this. Doing things like size() or oop_iterate() on from-space objects 
sounds like there's an abstraction/load-barrier missing somewhere. I can 
understand if the GC sometimes needs to fiddle with from-space objects, 
but that should be a Shenandoah internal thing and shouldn't have to 
leak out into BarrierSet, etc.

Btw, once your conversion to to-space invariant is complete (is it?), 
I'm hoping we can start to cleaning out all the stuff we no longer need 
from BarrierSet and the Access API.

cheers,
Per

> 
> I am wondering if a simple BarrierSet::obj_klass() might be
> useful/feasible for such cases? I can work around some of those
> occurances by calling variants that take a Klass* parameter instead,
> and resolve that internally in the GC. But not always. For example, I
> can call oopDesc::size_given_klass() instead. However, that blows up
> later because objArrayKlass::obj_size() is asserting klass()-
>> is_objArrayKlass() or something like that later...
> 
> Ideas?
> 
> Roman
> 
>> On 4/12/19 8:23 AM, Roman Kennke wrote:
>>>
>>> Am 12. April 2019 07:18:41 MESZ schrieb Per Liden <
>>> per.liden at oracle.com>:
>>>> Hi Roman,
>>>>
>>>> On 04/11/2019 10:58 PM, Roman Kennke wrote:
>>>>> An upcoming feature in Shenandoah requires that GC can
>>>>> intercept
>>>> loading
>>>>> the Klass* of an object. I'd like to introduce a GC interface
>>>>> for
>>>> that.
>>>>
>>>> Could you please give some more insight into what this feature is
>>>> and
>>>> why it needs to intercept these loads?
>>>
>>> Oh yeah, apparently it was late last night. ;-) We want to
>>> eliminate Shenandoah's extra word for the forwarding pointer and
>>> stick it in the Klass* word for forwarded objects, with a little
>>> bit of encoding to distinguish a forward pointer from a Klass*.
>>> Therefore we need to see Klass* loads to check that encoding and
>>> possibly load the Klass* from the forwardee instead.
>>
>> Maybe I'm missing something, but didn't you just switch to having a
>> to-space invariant? What runtime code can get hold of a an oop
>> pointing
>> into from-space and then try load its klass?
>>
>> cheers,
>> Per
>>
>>> Roman
>>>
>>>> thanks,
>>>> Per
>>>>
>>>>> This change covers the runtime part.
>>>>>
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8222281
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8222281/webrev.00/
>>>>>
>>>>> The change takes the 3 variants of oopDesc::klass() and funnels
>>>>> them
>>>>> through the Access API to be intercepted by the GC in any way
>>>>> it
>>>> wants.
>>>>> Behaviour- and performance-wise it should be identical
>>>>> (assuming the
>>>>> compiler can make sense of the Access API).
>>>>>
>>>>> I see that there might be an opportunity here to make the if
>>>>> (UseCompressedClassPointers) check pre-resolved, but I don't
>>>>> know how
>>>> to
>>>>> do that. I also don't really see how that is supposed to work
>>>>> for
>>>> loads
>>>>> and stores wrt UseCompressedOops either: in order to select the
>>>> proper
>>>>> functions on first call, and subsequently go through that
>>>>> selected
>>>>> function, it would have to be done in the *_init() functions.
>>>>> But I
>>>>> don't see any selection code there. Instead, it seems to be in
>>>>> PreRuntimeDispatch? I must be missing something. I left the
>>>>> selection
>>>> in
>>>>> the raw implementation. Maybe we want to sort this out?
>>>>>
>>>>> <rant>
>>>>> I must say that I fought with myself whether or not I should
>>>>> add to
>>>> the
>>>>> madness that is the Access API. What should have been a
>>>> 1-line-addition
>>>>> to an API (e.g. BarrierSet) turned out to become:
>>>>>
>>>>>     7 files changed, 86 insertions(+), 20 deletions(-)
>>>>>
>>>>> and two days of work. And god forbid we ever have to change or
>>>>> even
>>>> fix
>>>>> anything there.
>>>>>
>>>>> I was about to just not do it in Access API at all, but
>>>>> somewhere
>>>> else
>>>>> instead, but then I did not want to introduce a schism there,
>>>>> so I
>>>> bit
>>>>> the bullet. Maybe we should consider to turn this into a proper
>>>>> C++
>>>>> interface instead? This is just unmaintainable madness.
>>>>> </rant>
>>>>>
>>>>> Testing: tier1 fine. Will submit into jdk/submit shortly, works
>>>>> with
>>>> the
>>>>> Shenandoah prototype that I have here (iow, the API is good)
>>>>>
>>>>> Can I please get a review?
>>>>>
>>>>> Thanks,
>>>>> Roman
>>>>>