RFR: JDK-8222545: Safe klass asserts

[Roman Kennke]

Am 18.04.19 um 17:34 schrieb Stefan Karlsson:
> On 2019-04-18 17:29, Roman Kennke wrote:
>>
>>
>> Am 18.04.19 um 16:34 schrieb Stefan Karlsson:
>>> On 2019-04-18 15:13, Roman Kennke wrote:
>>>>>>>> To add a little more detail, I could move the change up into 
>>>>>>>> is_objArray(), but I don't want to expose it to any non-assert 
>>>>>>>> paths. Therefore I could do 2 different impls there, guarded by 
>>>>>>>> #ifdef ASSERT but I don't think it's a good idea to behave 
>>>>>>>> differently under ASSERT, that kindof defeats the point of 
>>>>>>>> assert, right?
>>>>>>>>
>>>>>>>> What do you think ?
>>>>>>>
>>>>>>> I don't follow your argument. Under asserts you need to access 
>>>>>>> the klass pointer "safely" but otherwise you do not. So there are 
>>>>>>> two behaviours related to accessing the klass pointer anyway. I'd 
>>>>>>> rather see that encapsulated in the accessor.
>>>>>>>
>>>>>>> I assume it's not just asserts but any debug only code that wants 
>>>>>>> to access the klass pointer.
>>>>>>
>>>>>> In general, for any runtime calls into oopDesc::klass() the access 
>>>>>> should be safe. The acrobatics is only necessary for *GC-internal* 
>>>>>
>>>>> This is the part I don't quite understand, and goes back to my 
>>>>> initial question. Why are you doing these operations on from-space 
>>>>> objects? I'm thinking you should be in a position in the GC to make 
>>>>> sure this can never happen. If you need to do that in the GC (which 
>>>>> is fine), then the GC could apply a "resolve" function to get the 
>>>>> to-space object, and call size() (or whatever) on that object. This 
>>>>> shouldn't have to leak out of the GC, right?
>>>>
>>>> It is a problem when we are about to evacuate an object. Then we 
>>>> need to know its size in order to allocate and copy an appropriate 
>>>> chunk. The problem is that this part is racy: two threads (e.g. two 
>>>> Java threads via barrier, or one Java thread vs one GC thread) might 
>>>> compete over this: both would create a copy of the object, but 
>>>> ultimately only one would succeed (by CASing the fwd pointer). 
>>>> Therefore, getting hold of the object size is racy, by design, and 
>>>> this requires to resolve the _klass. Now, we can do that ahead of 
>>>> time, and call oopDesc::size_given_klass() and all would be good, 
>>>> except that size_given_klass() asserts that the object is indeed of 
>>>> the given klass, and hence fetches _klass again, which, at this 
>>>> point, is racy. Solving this inside the GC would require to 
>>>> basically copy all the machinery to get hold of object size into the 
>>>> GC. Are you asking me to do that?
>>>
>>> Other GCs store forwarding pointers in the mark word. See 
>>> oopDesc::forward_to and friends. Could you do the same and get rid of 
>>> this problem?
>>>
>>
>> No. Other GCs store the fwd pointer there, but only during a pause, 
>> and while possibly stashing the mark word somewhere else in the meantime.
>>
>> We need to do it outside of GC pauses, plus we need a way (bit) to 
>> indicate what it actually is (fwd pointer or Klass*). The mark word is 
>> already badly overloaded and also accessed much more often and in 
>> critical paths (e.g. locking), while the Klass* is basically 
>> immutable, and has the lowest 3 bits free (when running with 
>> -UseCompressedClassPointers, which would be enforced by Shenandoah). 
>> Using the Klass* slot is therefore the simplest and most efficient 
>> place to keep the fwd pointer. Attempting to use the mark word would 
>> require much more barriers and cause more overhead to manage it.
> 
> Are you sure? Remember, the object is in from-space and no thread is 
> allowed to change it, except the threads that are copying out of the 
> from-space.

Right. But we still need one bit in it to differentiate between fwd ptr 
and regular mark word. And installing the fwd pointer concurrently with 
locking seems more of a horror story than dealing with an otherwise 
immutable Klass*.

In any case, I guess we could do without any GC interface changes by 
dropping the asserts in size_given_klass() if you think that is 
reasonable, also avoid the extra asserts in klass() and friends (even 
though I would prefer to have some way to assert sanity there...), plus 
the changes for JDK-8222537 which are arguably an improvement in any case.

Roman