RFR: JDK-8222545: Safe klass asserts

[Roman Kennke]

Am 18. April 2019 19:40:44 MESZ schrieb Per Liden <per.liden at oracle.com>:
>On 04/18/2019 07:15 PM, Roman Kennke wrote:
>> Am 18.04.19 um 18:47 schrieb Per Liden:
>>>
>>> On 04/18/2019 06:21 PM, Roman Kennke wrote:
>>>>
>>>>
>>>> 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 
>>>
>>> We already have such bits, and oopDesc::forward_to_atomic() will set
>
>>> them for you. Aren't they enough?
>>>
>>>> locking seems more of a horror story than dealing with an otherwise
>
>>>> immutable Klass*.
>>>
>>> With a to-space invariant, I can't see how this can happen 
>>> concurrently with locking.
>> 
>> Let's say we have one thread (1) which wants to evacuate an object,
>and 
>> another which does some locking operation on it (2). (1) would 
>
>But how did that second thread even get hold of that from-space object 
>in the first place? That thread would also _first_ race to evacuate it,
>
>_then_ try to lock it, when it's in to-space.

OK, you are right. My brain is apparently still half-stuck in weak invariant mode. Let me try that and see if this works without new GC interfaces ;-)

Thanks,
Roman




>cheers,
>Per
>
>> optimistically copy the object, then attempts to install the
>forwarding 
>> pointer into header, while (2) modifies the header. Which means 1.
>oops, 
>> the header changed, need to CAS again, 2. How to ensure consistency
>in 
>> the to-space object after evac ? I suppose this can be solved by a 
>> reasonable protocol, but even just thinking about it gives me a 
>> headache, while the same on an immutable Klass* or external brooks 
>> pointer is trivial.
>> 
>> Also, I don't understand why we even need to bikeshed this. All I am 
>> proposing is:
>> 1. A sanity assert in klass() and friends which hurts nobody. We
>could 
>> actually make this a little nicer and check klass->is_klass(), and
>add 
>> the GC-hook CollectedHeap::is_klass() there, similar to what we
>already 
>> do in oopDesc::is_oop().
>> 2. Making the asserts in path from size_given_klass() reliable in
>light 
>> of concurrent modification. Instead of dropping those asserts, I
>would 
>> actually like to strengthen them and not only assert
>obj->is_XYZArray() 
>> but even assert obj->klass() == klass, with the appropriate
>precautions 
>> needed for GC/Shenandoah.
>> 3. Avoid calling klass() twice in typeArrayOop::size() (JDK-8222537) 
>> which is actually a (minor) performance improvement.
>> 
>> Is this too much to ask?
>> 
>> Removing an otherwise good assert, and/or not allowing an assert in 
>> klass() seems to hurt our debugging capabilities. This doesn't make 
>> sense to me.
>> 
>> Roman

-- 
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.