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

[Vladimir Kozlov]

Calculation of orig_nonstatic_field_size variable is also wrong. It does 
not take into account fields padding.
But it is not important. We can skip PrintCompactFieldsSavings code if 
class or its fields are contended because we don't care about compaction 
in such case.

Vladimir

On 5/13/13 2:01 PM, Vladimir Kozlov wrote:
> 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.
>>
>
>