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

[Aleksey Shipilev]

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?

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.