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

[Vladimir Kozlov]

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)

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.
>