RFR (XS): JDK-8040977: G1 crashes when run with -XX:-G1DeferredRSUpdate

[Bengt Rutisson]

Hi Thomas,

Sorry for the very late reply.

I think the dependency between G1ParScanClosure is still very awkward, 
but I think your change is a step in the right direction. Thanks for 
fixing this.

Reviewed.
Bengt


On 2014-05-27 12:39, Thomas Schatzl wrote:
> Hi Bengt,
>
>    thanks for the review.
>
> On Wed, 2014-04-30 at 12:50 +0200, Bengt Rutisson wrote:
>> Hi Thomas,
>>
>> On 2014-04-22 14:56, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     can I have reviews for this change? It fixes wrong order of
>>> declaration of members of G1ParScanThreadState that causes crashes when
>>> G1DeferredRSUpdate is disabled.
>>>
>>> The change is based on the changes for 8035400 and8035401 posted recently.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8040977
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8040977/webrev/
>> I realize that this fixes the code but I would really appreciate a more
>> stable way of handling the dependencies.
>>
>> As it it now we end up calling methods on a G1ParScanThreadState
>> instance while we are setting it up. This seems broken to me and will
>> probably lead to similar initialization order issues again. Best would
>> be to not pass "this" to the constructor of G1ParScanClosure and instead
>> manage the circular dependency between G1ParScanClosure and
>> G1ParScanThreadState more explicitly after they have both been properly
>> set up.
>>
>> Second best would be to at least pass the worker id/queue num as a
>> separate parameter to avoid having to call methods on an uninitialized
>> object.
> I fixed this implementing the former idea. Also added some
>
> New webrev at
> http://cr.openjdk.java.net/~tschatzl/8040977/webrev.1/
>
> (Sorry, I already had merged the changes before making a diff webrev -
> however, most changes in the VM code have been redone anyway. The test
> case stayed the same).
>
> Thanks,
>    Thomas
>