RFR (M): 8004687: G1: Parallelize object self-forwarding and scanning during an evacuation failure

[Jon Masamitsu]

On 07/20/2015 07:16 AM, Thomas Schatzl wrote:
> Hi Jon,
>
>
> On Fri, 2015-07-17 at 12:57 -0700, Jon Masamitsu wrote:
>> Thomas,
>>
>> Changes look good.
>    thanks for the review.
>
>> Would it work to change
>>
>> OopAndMarkOop
>> to
>> OopAndOldMarkOop
>>
>> and change the constructor to
>>
>> 866     OopAndMarkOop(oop obj) : _o(obj), _m(markOop(obj))
>>
>> so that OopAndOldMarkOop specifically saves the oop and it's
>> markword rather than just an oop and (not necessarily related)
>> markword?
>    the only problem I see is that we earlier (8006025) changed the code
> to explicitly not re-read the markoop from the obj multiple times. The
> markoop is volatile, so it prevents some optimizations which we want
> here.
>
> It may not be a huge problem or one at all in this particular case,
> depending on where the OopAndOldMarkOop is instantiated, but requires
> checking.

If I understand this comment correctly, yes it does require
checking but just as it requires checking when the read of the
markoop was done (the one read of the markoop).  It seemed
to me to be modestly more clear with now that there was
an OopAndMarkOop (renamed OopAndOldMarkOop or
OopAndPreservedMarkOop which I like even better) but
sometimes less code is better (i.e., simpler not to use
OopAndMarkOop data structure) so your call.


>
> I put a webrev with pure renaming of the struct at
> http://cr.openjdk.java.net/~tschatzl/8004687/webrev.1/ (diff webrev:
> http://cr.openjdk.java.net/~tschatzl/8004687/webrev.0_to_1/).

The diff (0_to_1) seem to be only the deletion of some code,
not a renaming but maybe a moot point now if you still
like explicitly reading the value of the markoop that you
want to capture.

Jon

>
> I do not mind either version.
>
>> An enhancement to be considered now or later. You have
>> OopAndMarkOop now.  Could you pass an OopAndMarkOop
>> around instead of  an oop and a markword pair.  For
>> example, starting with
>>
>> 201 oop copy_to_survivor_space(InCSetState const state, oop const obj,
>> markOop const old_mark);
>>
>> substitute passing an OopAndMarkOop in place of the obj and old_mark.
> Prototyping this it does not seem to be huge gain. The use and passing
> of a struct is only really advantageous in a few places (in
> copy_to_survivor_spaces()). In many places the existing code manipulates
> the old object and old mark quite a bit, adding a "obj_and_mark" (if we
> call the parameter obj_and_mark) prefix in quite a few places only.
>
> I put that change at
> http://cr.openjdk.java.net/~tschatzl/8004687/webrev.refactor/.
> Particularly lines 250-265 in g1ParScanThreadState.cpp seem to be more
> obfuscated than before. There is opportunity to move the entire mark
> update method into OopAndOldMarkOop, but unless there is something I
> omitted, I would like to defer it (and the necessary performance
> checking work) to a later point.
>
>> Reviewed.
> Thanks,
>    Thomas
>