RFR (M) #5 CR 8003985: Support @Contended annotation

[Vladimir Kozlov]

On 1/11/13 3:11 PM, Aleksey Shipilev wrote:
> On 01/12/2013 02:10 AM, Vladimir Kozlov wrote:
>> I mentioned this because for previous your changes "8004330: Add missing
>> Unsafe entry points" I had to do it for you. You should make life of
>> your sponsor better and not worse, we have a lot of other things to do.
>
> OK, sounds fair. I wish you had this explanation in 8004330 timeframe. I
> was under the false impression this is how we do the coordinated
> HotSpot-JDK change.

We (VM group) can push jdk changes again using hotspot-main/jdk repo as 
with 8004330 to synchronize push.

I just want a separate webrev for lib changes to let lib guys know what 
is coming to them.

Also VM push process (JPRT) does not do automatic push into jdk code 
together with VM changes - it is 2 different jobs. So we have to 
separate Hotspot and JDK changes.

>
>> As Joe responded we can use the same bug id or create subCR. I rises
>> this question because of long conversation thread this week about jdk
>> changes for, again, 8004330 when library guys did not know what happened
>> with the changes.
>
> Ok, you want me to ask corelibs guys to push JDK side of changes on
> their own? You will have to wait with HotSpot update then, because that
> otherwise it would render jtreg test non-compilable. I left @Contended
> in HotSpot webrev, so that is still separately compilable:
>    http://cr.openjdk.java.net/~shade/8003985/webrev.vm.02/

This looks better. Please, fix copyright year in Contended.java

>
> I will post JDK changes for reviews shortly.

Thanks,
Vladimir

>
>>> Beefing up ContendedPaddingWidth when larger cache line was detected is
>>> fine. But, will user setting be favored this way? I would like to be
>>> able to set it lower than the cache line size for perf testing.
>>
>> You can accomplish this with:
>>
>> if (FLAG_IS_DEFAULT(ContendedPaddingWidth) &&
>>      (cache_line_size > ContendedPaddingWidth))
>>      ContendedPaddingWidth = cache_line_size;
>
> Added. Debug printing is also added.
>
>> You don't need to put details, just what you said is enough:
>>
>> "Contended group defines the equivalence class over the fields: the
>> fields within the same contended group are not inter-padded. The only
>> exception is default group, which does not incur the equivalence, and so
>> requires intra-padding."
>
> Added this at the code points which confused you.
>
>>>> Also why you padding after fields again? You did padding before fields
>>>> already? You can wasting space at the end of object or in between super
>>>> and subclass fields.
>>>
>>> Yes, and that is expected: the padding in the end saves us from false
>>> sharing against either subclass fields, or the header/fields of adjacent
>>> object. The adjacent object can be unpadded (generally we don't even
>>> know what that object is).
>>
>> Add comment to help a next developer who will touch this code.
>
> Added.
>
>>>
>>>> In vmSymbols.hpp Contended signature should not be in "error klasses"
>>>> group. Separate it by comment.
>>>
>>> Cosmetics; can be updated before the push.
>>
>> But you do that, sponsor will not do that.
>
> Yeah, I didn't mean sponsor should do it. Rather, I was pointing this is
> a trivial change which could be fixed without a review.
>
> Fixed, I had also consumed AbstractOwnableSynchronized in the same group.
>
>>>
>>>> In the test change copyright year to 2013. Also @summary should be bug's
>>>> "Support Contended Annotation - JEP 142"
>>>
>>> Ditto. Cosmetics; can be updated before the push.
>>
>> Same.
>
> Fixed.
>
> -Aleksey.
>