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
