RFR(S): 8173013: JVMTI tagged object access needs G1 pre-barrier

[sangheon]

Hi Thomas,

On 02/03/2017 05:51 AM, Thomas Schatzl wrote:
> Hi Sangheon,
>
> On Thu, 2017-02-02 at 20:00 -0800, sangheon wrote:
>> Hi David,
>>
>> Thank you for looking at this!
>>
>> On 02/02/2017 05:20 PM, David Holmes wrote:
>>> Hi Sangheon,
>>>
>>> On 3/02/2017 5:11 AM, sangheon wrote:
>>>> Hi all,
>>>>
>>>> Could I have some reviews for this change that adds G1 pre-
>>>> barrier?
>>>>
>>>> JvmtiTagHashmapEntry has a bare oop member and this is a weak
>>>> reference.
>>>> So any place that allows this oop to 'escape' needs the G1 pre-
>>>> barrier.
>>>> JvmtiEnv::GetObjectsWithTags() provides such an escape path.
>>>>
>>>> For G1, in order to maintain the SATB invariants, reading the
>>>> referent
>>>> of a weak reference must ensure the referent is marked alive.
>>>>
>>>> So this proposal includes adding the pre-barrier at
>>>> TagObjectCollector::do_entry(JvmtiTagHashmapEntry* entry) which I
>>>> see
>>>> the only place interacts(except 'peek' operations) with the bare
>>>> oop
>>>> member.
>>> Pardon my GC ignorance but it seems odd to me that this barrier is
>>> inserted immediately before we create a local JNIHandle. Won't the
>>> JNIHandle ensure the object is seen as live?
>> Unfortunately it isn't.
>>
>> If we are using G1 and accessing the value of the referent field in a
>> reference object then we need to register a non-null referent with
>> the SATB barrier.
>>
>> In this code path, for example:
>>
>> (1) Object x is tagged.
>> (2) x becomes unreachable, with the only remaining reference being
>> the weak reference in the tag hashmap.
>> (3) Concurrent GC starts and has completed marking, but has not yet
>> entered the remark pause where reference processing occurs.
>> (4) GetObjectsWithTags is used to obtain x. As reference processing
>> has not yet run, x is still present in the tag hashmap. x remains
>> unmarked, because there is no read(SATB) barrier on that access.
>> (5) GC remark pause runs reference processing, determines x is dead,
>> so clears the tag entry, and reuses the space previously occupied by
>> x.
>> (6) The GetObjectsWithTags result now refers to a dead and reclaimed
>> x.
>> A crash may follow.
>> (From Kim Barrett's note)
>>
>> FYI, there are similar treatments already for this case.
>> Plz, search for "G1SATBCardTableModRefBS::enqueue", especially
>> Unsafe_GetObject().
>>
>> I added this comment at the patch.
>> Webrev: http://cr.openjdk.java.net/~sangheki/8173013/webrev.1
> 1539         // We could be accessing the referent field in a reference
> 1540         // object. If G1 is enabled then we need to register non-
> null
> 1541         // referent with the SATB barrier.
>
> I do not think this comment is complete, or at least I do not see why
> the object in question needs to be a "referent field in a reference".
> Maybe you meant that the tag map entry acts similar to the referent of
> a j.l.ref.WeakReference, so we need to handle it the same as when a
> thread accesses a weak reference in normal java code (i.e. the read
> barrier).
Thank you for correctly understanding what I wanted to say.

>
> As the problem description above indicates, this can occur with any
> reference to an object where the tag map has the only (implicitly weak)
> reference to as far as I can see.
>
> I.e. maybe something like:
>
> "The reference in this tag map could be the only (implicitly weak)
> reference to that object. If we hand it out we need to keep it live wrt
> SATB marking similar to other j.l.ref.Reference referents."
I like this comment.
If other reviewers don't have better suggestion, I will upload the 
revised patch with this.

Thanks,
Sangheon


>
> Probably others can provide a better description.
>
> Otherwise it seems good.
>
> esses
>
> Thanks,
>    Thomas