RFR (S) CR 8014886: @Contended fields can overrun oop maps

[Daniel D. Daugherty]

On 5/21/13 3:03 AM, Aleksey Shipilev wrote:
> Thanks Vladimir!
>
> The updated webrev is here:
>    http://cr.openjdk.java.net/~shade/8014886/webrev.02/

hotspot/src/share/vm/classfile/classFileParser.cpp
     I'm OK with the switch back to the original size calculation
     (now in max_nonstatic_oop_maps). I'm also happy with the
     new asserts.

hotspot/test/runtime/contended/OopMaps.java
     Thanks for verifying that the new test fires the new asserts

For testing you listed:

 >  - new regression test added, fails on new asserts
 >  - Linux x86_64/fastdebug runtime/contended/ tests
 >  - full JPRT cycle against hotspot-rt

which does not include the vm.quick testsuite which is minimum
testing required for RT_Baseline pushes unless the change is
trivial. I don't think anything associated with the @Contended
work can be considered trivial so I would like to see vm.quick
testing done on two platforms. One of those platforms should
be Windows since it is known to be quirky with oopmaps.


Just to review the bidding on the already filed or promised new bugs:

0) refactor the entire _count handling during the class field
    layout cleanup (JDK-8014878)

1) prune the "+ 1" from the max_nonstatic_oop_maps or add a
    comment to justify why it is there

2) current code allocates oopmap per contended oop fields even fields
    in one group (not separated by padding) - tighten this up

3) has_nonstatic_fields is also wrong (JDK-8014964)

Do you have a planned time frame or time frames for these bugs?

Dan





>
> On 05/21/2013 07:58 AM, Vladimir Kozlov wrote:
>> Is not has_nonstatic_fields also wrong?
> Yes, it is. I haven't fixed this here for three reasons:
>   a) this is not about oop maps
>   b) tracked down the impact through the HS code, seems minimal
>   c) because of (b): I'm still trying to get proper regression test for
> has_nonstatic_fields ready
>
> I will post another CR for fixing that one up.
>
>> Rename: max_oop_maps -> max_nonstatic_oop_maps.
> Done.
>
>> For FieldsAllocationStyle=2 (default) we do special oop fields layout
>> for super and subclass to have only one oopmap. You need to add non-oop
>> fields (class in your test has only oop fields) to test different layouts.
> Done. I had also the super-class to get the oops aligned in the oop chunk.
>
>> Next added mess to already complicated code:
>>
>> 3188   unsigned int nonstatic_double_count =
>> fac->count[NONSTATIC_DOUBLE] - fac_contended.count[NONSTATIC_DOUBLE];
>> 3189   unsigned int nonstatic_word_count   = fac->count[NONSTATIC_WORD]
>>    - fac_contended.count[NONSTATIC_WORD];
>> 3190   unsigned int nonstatic_short_count  = fac->count[NONSTATIC_SHORT]
>>   - fac_contended.count[NONSTATIC_SHORT];
>> 3191   unsigned int nonstatic_byte_count   = fac->count[NONSTATIC_BYTE]
>>    - fac_contended.count[NONSTATIC_BYTE];
>> 3192   unsigned int nonstatic_oop_count    = fac->count[NONSTATIC_OOP]
>> - fac_contended.count[NONSTATIC_OOP];
>> At least add comment that these counts are for not contended fields.
> Already taken care of in the baseline: there is a terse comment. I will
> clean up that mess during further cleanup work. The regression tests
> like this one will help to build enough safety net for the refactoring.
>
> -Aleksey.
>