RFR (S): CR 8012939: @Contended doesn't work correctly with inheritance

[Vladimir Kozlov]

On 5/13/13 12:43 PM, Aleksey Shipilev wrote:
> On 05/13/2013 11:29 PM, Vladimir Kozlov wrote:
>> I meant to make it local to use place with renaming if needed.
>
> Ah, you mean at the very first change. I can do that, thanks.

I meant both places. You can simple remove

3148   int next_nonstatic_type_offset;

and add 'int' in use places because they are in different scopes.

>
>>>> I think the fix is not right. If super is contended the nonstatic size
>>>> should contain the padding already:
>>>>
>>>> 3134   int nonstatic_field_size = _super_klass() == NULL ? 0 :
>>>> _super_klass()->nonstatic_field_size();
>>>>
>>>> Why it is not?
>>>
>>> Because nonstatic_field_size was actually missing the trailing padding.
>>
>> Why? Do you mean next codes did not add padding for super class:
>>
>> 3189   // class is contended, pad before all the fields
>> 3190   if (parsed_annotations->is_contended()) {
>> 3191     first_nonstatic_field_offset += pad_size;
>> 3192   }
>> 3193
>>
>> 3586   // Entire class is contended, pad in the back.
>> 3589   if (parsed_annotations->is_contended()) {
>> 3590     notaligned_offset += pad_size;
>> 3591   }
>
> There are two parts to this story: one is adding the padding for the
> class, which obviously works, and the second one is adding the padding
> for the field, which is missing. See the regression test there: the

Then why the next code does not work in such case?:

3482     // if there is at least one contended field, we need to have
pre-padding for them
3483     if (nonstatic_contended_count > 0) {
3484       next_nonstatic_padded_offset += pad_size;
3485     }


> field from the super-class and the field in subclass are not separated
> by any padding, even though both are @Contended.
>
>>> See the assert before, it is obviously not correct: instance_size misses
>>> the padding, and we need to add it up in the right hand side.
>>
>> Yes, assert was wrong.
>>
>>>
>>>> Also your fix does double padding:
>>>>
>>>> 3588   if (parsed_annotations->is_contended()) {
>>>> 3589     notaligned_offset += pad_size;
>>>> 3590   }
>>>>
>>>> 3598              + (parsed_annotations->is_contended() ? pad_size :
>>>> 0))/heapOopSize);
>>>
>>> Yes it does, but this is not introduced by the fix. The double padding
>>
>> You added line 3598. As result it adds class padding 2 times at the end.
>
> Ah, this line is confusing. See the chunk you had already posted?
>
>   3189   // class is contended, pad before all the fields
>   3190   if (parsed_annotations->is_contended()) {
>   3191     first_nonstatic_field_offset += pad_size;
>   3192   }
>   3193
>
> What line 3598 actually does is adds the leading padding *back* so we
> can calculate the proper instance size, because
> first_nonstatic_field_offset is already not at the start of the fields.

So HERE !!! is the main problem, first_nonstatic_field_offset does not
include class pre-padding. It was totally not clear from your changes.
And my question above about fields padding still stay, why it did not
add padding for Contended fields?

I think the better fix would be to use separate variable which includes
pre-padding and use it in nonstatic_field_size and instance_size
calculation.

Vladimir

>
>>> is due to this case:
>>>
>>>    @Contended
>>>    class C {
>>>       @Contended
>>>       int field;
>>>    }
>>
>> The bug report is not for this case.
>
> Sure, I'm just demonstrating how the double padding arises. That was a
> deliberate choice for the basic implementation.
>
>> Let see.
>>
>> Next expression should give correct nonstatic_field_size with class
>> padding for super class:
>>
>> 3134   int nonstatic_field_size = _super_klass() == NULL ? 0 :
>> _super_klass()->nonstatic_field_size();
>>
>> It should be (272-head_size)/heapOopSize for bug case. If it is not,
>> then that is the bug.
>
> I had the printed debugging to demonstrate the nonstatic_field_size was
> the culprit, because we accidentally chomped off the leading field
> padding the super-class by subtracting already "off"
> first_nonstatic_field_offset.
>
> Do you want me to rework this to make the intent clearer?
>
> -Aleksey.
>