RFR (XS): 7114095: G1: assert(obj == oopDesc::load_decode_heap_oop(p)) failed: p should still be pointing to obj

Srinivas Ramakrishna ysr1729 at gmail.com
Fri Dec 2 07:51:28 UTC 2011


On Thu, Dec 1, 2011 at 10:47 AM, John Cuthbertson <
john.cuthbertson at oracle.com> wrote:

> Hi Ramki,
>
> Response inline....
>
>
> On 12/01/11 01:08, Srinivas Ramakrishna wrote:
>
>> Hi John -- I am wondering if we should simply prevent the second closure
>> application if we have already
>> done it once. That is easy to set-up. Of course such a test penalizes all
>> collectors when they scan Reference objects, but perhaps the cost of the
>> test
>> will be lost in the noise of all the other test-logic when scanning
>> Reference objects...
>>
>
> Yes this is straight forward to do - but as you said it will add an
> additional clause to the if-condition that's around the second application.
> I have verified that this does seem to fix the problem. If I go with this
> approach - do you foresee any issues with skipping the second application
> of the closure to the discovered field? I don't think there is but it's
> worth asking the original author. :) I went with the fix that's in the
> webrev because it only touches G1 code.


The only time we will be skipping the second application is when it's
already been done in the earlier clause,
which fires only for G1. So it will be skipped only in the case of G1 and
then only in the case of closures
that want to apply to the discovered field regardless. So I don't see any
correctness issue (i.e. brittleness)
arising from this approach. The only possible concern was, as we both seem
to agree, in the possible
performance impact of the additional boolean clause in the if-test (and
it's initialization). I suspect
the perf impact will be in the noise.

Thus it would seem as though the only reason to prefer one or the other
would be based on "taste".


>
>  But since G1 deals with multiple scans anyway, perhaps this is not worth
>> worrying over.
>>
>> I also have some related questions: what forces G1 to have to deal with
>> the multiple scan possibility anyway?
>> I had previously incorrectly assumed that G1's remembered set scanning
>> also avoids multiple scans of any
>> reference...
>>
>
> During RSet scanning - the closure claims a group of cards. That along
> with a couple of "is_in_cset" tests seem to be enough to prevent multiple
> scans of the same reference field (i.e. pushing the same reference field on
> to the _refs_to_scan queue more than once).


Good.


>
>
>  In the scenario that you outlined, when the second thread processes the
>> reference the second
>> time, how does it determine that it should do nothing? (I guess I am
>> asking for a pointer to the
>> code that does the processing of the stack elements and does the
>> recursive copying.)
>>
>
> The failing assert indicates that reference fields should only be pushed
> on to the _refs_to_scan queue once and mostly that seems to be the case.
> After an object is evacuated, the worker that successfully installs the
> forwarding pointer iterates over the reference fields of the evacuated
> object, applying the G1ParScanClosure. Since this is done by the thread
> that claims the object - we shouldn't see multiple pushes of the same
> reference field unless the closure is applied to the reference field
> multiple times.  The discovered field of reference objects is the only
> field I've seen being pushed twice.
>
> As reference fields are popped from the _refs_to_scan queue, a copy
> closure is applied to them. This takes place in G1ParScanThreadState::deal_
> **with_reference(). I believe the call in question comes from
> G1ParScanThreadState::trim_**queue(). The copy closure dereferences the
> field and first checks that the referenced object is in the CSet before
> attempting to evacuate the object. This code is in
> G1ParCopyClosure::do_oop_work(**) and G1ParCopyHelper::copy_to_**survivor_space()
> in g1CollectedHeap.cpp.
>
> The evacuation code has to handle multiple reference fields that point to
> the same object (an already evacuated object will not pass the "is_in_cset"
> check), and it looks like it can handle seeing the same reference field
> twice. The worst that I think can happen is that the reference field may be
> updated twice (to point to the same evacuated object).
>
> A while back, the evacuation code took a fourth template parameter - a
> boolean that indicated whether the "is_in_cset" check could be skipped,
> i.e. it was assumed that the closure would only be applied to objects in
> the collection set. If we still had that code then we wouldn't be able to
> handle this scenario.
>
> Does this answer your questions?
>

Very nicely. Thanks for the crystal clear explanation!!

I'll let you decide which of the two approaches looks better to you. Left
to myself I might be
slightly inclined to go with the new solution of never letting a reference
appear twice in the
"gray list" by modifying the refernce object scanning.
But I'm fine with either solution based on how you, Tony etc. feel about it.

thanks John!
-- ramki



>
> Thanks,
>
> JohnC
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20111201/80ec793b/attachment.htm>


More information about the hotspot-gc-dev mailing list