RFR (S) CR 8014964: @Contended breaks has_nonstatic_fields invariant

[Vladimir Kozlov]

On 5/21/13 9:06 AM, Vladimir Kozlov wrote:
>
> On 5/21/13 8:15 AM, Aleksey Shipilev wrote:
>> Thanks for the review!
>>
>> Updated webrev is here:
>>    http://cr.openjdk.java.net/~shade/8014964/webrev.02/
>>
>> On 05/21/2013 06:56 PM, Vladimir Kozlov wrote:
>>> In assert you don't need to test nonstatic_field_size > 0 because you
>>> tested it for 0 already so other tests will be executed only when it is
>>> != 0.
>>
>> Short-circuiting... fair enough. Fixed.
>
> Could you move third check on next line so you can see all on one screen?
>
>>
>>> I don't understand why you don't check has_nonstatic_fields when
>>> parsed_annotations->is_contended(). With the fix has_nonstatic_fields
>>> also includes contended fields so it does not matter if you have
>>> annotation or not.
>>
>> It does matter: for @Contended class without fields, we actually have
>> nonstatic_field_size > 0 (because it includes padding), but still no
>> fields. R3 class in regression test will fail the assert otherwise. I
>> added the comment about this before the assert. I'll keep in mind during
>> the cleanup whether something can be done to make @Contended class case
>> assertable as well.
>
> You are right. So parsed_annotations->is_contended() is true for any combinations of class and fields annotation. Right?
>
> Is it possible nonstatic_field_size > 0 without fields due to align_size_up() (for example, with compressed klass
> pointer in 64 bit VM)?
>
> An other issue. The assert is not precise for current class because from one hand it includes data from super and on an
> other hand it checks annotation for current class. Can you add second assert? Something like:
>
> assert(nonstatic_fields_count == 0 || parsed_annotations->is_contended() ||
>         (nonstatic_fields_end > nonstatic_fields_start)

Correction, checks should be reversed:

assert((nonstatic_fields_end == nonstatic_fields_start) ||
        parsed_annotations->is_contended() ||
        (nonstatic_fields_count > 0)

Vladimir

>
> This could be affected by alignment even more than your assert. Could you look?
>
> Thanks,
> Vladimir
>
>>
>> (In grand scheme of things, we might allow @Contended to skip generating
>> the padding if no fields are present, but this is messy to do with
>> current one-pass code).
>>
>> -Aleksey.
>>