RFR (XS): 8033922: G1: Back out 8033601 and go back to use the to-obj for chunked arrays.

[Thomas Schatzl]

Hi,

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.
> 
> http://cr.openjdk.java.net/~brutisso/8033922/webrev.00/
> 
> https://bugs.openjdk.java.net/browse/JDK-8033922
> 
> 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(), 
> _worker_id);
> 
> 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
pointer.

So I agree with undoing this change at least for now unless somebody
(Tony :)) comes up with a clearly better working solution.

Thanks,
  Thomas