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

[Vladimir Kozlov]

On 1/11/13 1:00 PM, Aleksey Shipilev wrote:
> On 01/11/2013 11:34 PM, Vladimir Kozlov wrote:
>> Please, file separate bug for jdk changes, create separate webrev and
>> send it to core-libs-dev at openjdk.java.net for review (CC to
>> hotspot-dev). And do it for future changes which involve jdk changes.
>
> (passive-aggressive mode on)
>
> Sorry, but this is very unsettling to hear on 10-th review cycle. Up
> until now I carefully jumped through the hoops of maintainers' opinions,
> but at this point my patience at all time low level. Anything else you
> guys have up your sleeve saved for the next round of reviews?

(passive-aggressive mode on)

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.

(passive-aggressive mode off)

>
> Will that "new" CR require the new spin of CCC... because, technically,
> that would be another CR, not the one CCC decision is acted upon? Will
> it be more convenient to dump the annotation stub from HotSpot change at
> all (which also throws away the jtreg test), and wait for this
> annotation to ship from regular jsr166 channel?

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.

>
> Please consider pushing this change in it's current form. This is needed
> for JDK8 concurrency work, and we can't really miss the integration
> deadline because of the cosmetic issues.
>
> (passive-aggressive mode off)
>
> Now to the comments:
>
>> You can set ContendedPaddingWidth lower and redefine it in
>> vm_version_<arch>.cpp:
>>
>>    if (cache_line_size > ContendedPaddingWidth)
>>      ContendedPaddingWidth = cache_line_size;
>
> 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;


>
>> On other hand 128 value is very conservative and should satisfy all
>> current platform VM run on. But you will wast java heap space.
>
> We can live with pessimistic value. That's the price we pay for dodging
> false sharing on the platforms with hardware prefetchers turned on.

OK.

>
>> There is no explanation in code what contending groups are. The only
>> comment I see:
>>
>> +          if (fs.contended_group() == 0) {
>> +            // special case, this default group is padded within
>>
>> Why other groups don't need padding between fields?
>
> That is by the definition of contended group. 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. I tempted not to go in the detail in HotSpot code,
> because that is (to be) explained in the @Contended Javadoc:
> http://gee.cs.oswego.edu/dl/jsr166/src/dl/sun/misc/sun/misc/Contended.html
> (which is, but the weird legalese reason, can not be committed in
> OpenJDK first, and then pushed to jsr166).

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

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

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

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

Vladimir


>
> -Aleksey.
>