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

[Joe Darcy]

On 1/11/2013 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?
>
> 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?

It is marginally acceptable to use the same bug id to push to different 
*repos*.  For example, the same bug id can be used to push into hotspot 
and jdk.  This has been done before many times and is okay with respect 
to jcheck.

If you want a separate bug id, create a subtask of 8003985 in JIRA to 
"push @Contended into jdk repo."

The ccc does *not* want to see the same version of this work resubmitted 
under a new bugid.

Cheers,

-Joe

>
> 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.
>
>> 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.
>
>> 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).
>
>> 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).
>
>> In vmSymbols.hpp Contended signature should not be in "error klasses"
>> group. Separate it by comment.
> Cosmetics; can be updated before the push.
>
>> 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.
>
> -Aleksey.