RFR (XS): 8033922: G1: Back out 8033601 and go back to use the to-obj for chunked arrays.
bengt.rutisson at oracle.com
Fri Feb 7 05:24:50 PST 2014
Thanks for the quick review!
Yes, there are ways to fix it, but I don't think it is worth the effort
since the original fix was just a fix to make G1 look more like the
other collectors. It was not a correctness or performance fix.
On 2/7/14 1:51 PM, Thomas Schatzl wrote:
> On Fri, 2014-02-07 at 13:29 +0100, Bengt Rutisson wrote:
>> Hi all,
>> Can I have a couple of reviews for backing out the changes for 8033601.
>> Those changes cause verification failures in our nightly testing.
>> Background from the CR:
>> The fix for 8033601 changed the code for chunked arrays to use the from
>> obj length as index rather than the to object.
>> This was nice since it made G1 look more like the other collectors.
>> However, another difference in G1 is that it writes the forward pointer
>> before it writes the to-object. That means that there are times when an
>> object is forwarded, but there is no to-object image that can be trusted.
>> One such place is
>> void G1ParCopyClosure<barrier, do_mark_object>
>> ::do_oop_work(T* p)
>> We can find that the object is forwarded, but it may not have been
>> marked yet. So, we end up calling mark_forwarded_object() which needs
>> the size to call _cm->grayRoot(to_obj, (size_t) from_obj->size(),
>> mark_forwarded_object() has a comment that tries to describe this situation:
>> // The object might be in the process of being copied by another
>> // worker so we cannot trust that its to-space image is
>> // well-formed. So we have to read its size from its from-space
>> // image which we know should not be changing.
>> _cm->grayRoot(to_obj, (size_t) from_obj->size(), _worker_id);
>> So, just as the comment say, we can not trust the to-space image.
>> However, after the change for 8033601 we can not trust the from-space
>> image either.
> I.e. the problem is that there is a small time window where another
> thread might read a wrong length field.
>> This seems like a show-stopper for the suggested change for 8033601 so
>> we need to back it out.
> An alternative would be to write/copy the object header (with the length
> field) in the to_obj before forwarding (the atomic forwarding should
> take care of memory ordering issues).
> This seems to be too much hassle though, as it has at least one
> disadvantage: it widens the opportunity for multiple threads working on
> the same object (potentially more undo).
> Another alternative could be to not continue with the rest of the body
> of G1ParCopyClosure<barrier, do_mark_object>::do_oop_work() if a thread
> in copy_to_survivor_space() looses the race for installing the forward
> So I agree with undoing this change at least for now unless somebody
> (Tony :)) comes up with a clearly better working solution.
More information about the hotspot-gc-dev