possible CMS problem with C2

[Y. S. Ramakrishna]

Hi Tom --

On 03/31/11 12:41, Tom Rodriguez wrote:
> While looking at the code we generate for a simple program I noticed that we were eliminating card marks with CMS in a illegal fashion.  For this simple program:
> 
> public class test {
>     static Object a1;
>     static Object a2;
>     static Object a3;
>     public static void main(String[] args) {
>         a1 = args;
>         a2 = args;
>         a3 = args;
>     }
> }
> 
> we generate this:
> 
> 000   B1: #     N1 <- BLOCK HEAD IS JUNK   Freq: 1
> 000     SAVE   R_SP,-72,R_SP
> 004     SET    precise klass test: 0x00843a50:Constant:exact *,R_L0     !ptr
> 00c +   SET    0xf92d4000,R_L2  !ptr
> 010 +   STW    R_I0,[R_L0 + #352]       ! ptr ! Field test.a3
> 014 +   STW    R_I0,[R_L0 + #348]       ! ptr ! Field test.a2
> 018 +   SRL    R_L0,#9,R_L1     ! Cast ptr R_L0 to int and shift
> 01c     STW    R_I0,[R_L0 + #344]       ! ptr ! Field test.a1
> 020 +   STB    #0,[R_L2 + R_L1] ! CMS card-mark byte 0
> 024     SETHI  #PollAddr,L0     ! Load Polling address
>         LDUW   [L0],G0  !Poll for Safepointing
>         RET
>         RESTORE
> 034 +   ! return
> 034
> 
> which happens to be ok because the STB happens last but we don't actually have enough dependences to ensure that we get this schedule.  We emit 3 separate StoreCMs for each field references and each one has a dependence on the store that it covers.  What's going wrong is that we're allowing StoreCM to participate in the the store elimination in StoreNode::Ideal so we end up with this:
> 
>  45     StoreP  ===  5  7  44  10  [[ 16  50 ]]  @precise klass test: 0x00843a50:Constant:exact+352 *, name=a3, idx=6;  Memory: @precise klass test: 0
> x00843a50:Constant:exact+352 *, name=a3, idx=6; !jvms: test::main @ bci:9
>  36     StoreP  ===  5  7  35  10  [[ 16 ]]  @precise klass test: 0x00843a50:Constant:exact+348 *, name=a2, idx=5;  Memory: @precise klass test: 0x008
> 43a50:Constant:exact+348 *, name=a2, idx=5; !jvms: test::main @ bci:5
>  25     StoreP  ===  5  7  24  10  [[ 16 ]]  @precise klass test: 0x00843a50:Constant:exact+344 *, name=a1, idx=4;  Memory: @precise klass test: 0x008
> 43a50:Constant:exact+344 *, name=a1, idx=4; !jvms: test::main @ bci:1
>  50     StoreCM ===  5  7  31  23  45  [[ 16 ]]  @rawptr:BotPTR, idx=Raw;  Memory: @rawptr:BotPTR, idx=Raw; !jvms: test::main @ bci:9
> 
> The store to a3 has a StoreCM and it has killed the StoreCMs for a1 and a2 but it doesn't have a dependence on it.  Since the slices are independent the a3 operations could be scheduled before the store to a1 and a2.  It may be that this is rare in practice and/or the local schedule tends to put the StoreCM last but it's clearly wrong.  G1 is safe from this because the StoreCMs are never close enough to be eliminated.
> 
> It's easy to fix but it might hurt CMS performance a bit.  It could also be done safely if the StoreCM could have dependences on multiple stores but since it's not using normal precedence edges I'm not sure how this would be implemented.
> 
> Does this sound like any issues that have been seen in the past?

If it's been seen in the past, it has never been identified
as stemming from this kind of optimization / missing dependencies.
I have heard people (Vladimir, Igor, John Cuthbertson) express the lack of
confidence in the existence of sufficient dependencies in some of the
card-mark eliminations before.

There was a time way back when Mike Paleczny and Ross Knippel
had fixed issues stemming from illegal (for CMS) elision of card-marks
in the case of CMS, and we had also taken care to add some
missing ones in the runtime for perm objects, but this
one is new to me.

Yes, please fix this; even though i imagine this will be rare
because (1) such scheduling may be rare (2) it's only when this
intersects with precleaning that this would manifest, and if
CMS collections are infrequent, the error will be rare (3) the
lack of a card-mark for the elided/rescheduled stores may be
masked by a different store to an adjacent object, making an
actual crash rarer still. But still, thanks for finding the
issue and fixing it! It would be interesting to see how much
of a difference in performance the new previously missing
dependencies will make to real codes/workloads.

-- ramki

> 
> tom