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

[John Rose]

On Dec 11, 2012, at 1:30 PM, Aleksey Shipilev wrote:

> On 12/11/2012 02:44 AM, John Rose wrote:
>> This is good.  I have some comments on it.
> 
> Ok, this is the updated webrev fixing most of John's comments:
>  http://shipilev.net/pub/jdk/hotspot/contended/webrev-7/
> 
> Changes are:
>  - ClassFileParser::_is_contended is gone
>  - merged "privileged" machinery from John's patch into ClassFileParser
>  - FieldInfo::type -> FieldInfo::allocation_type
>  - two JVM options: -XX:+EnableContendedOnBCP, and
> -XX:-EnableContendedOnCP added, so that @Contended is no-op for
> non-privileged classes until you flip the switch

The option names say too much, IMO.  I suggest removing the "OnBCP" part, and have -XX:-EnableContended disable @Contended everywhere.  I assume it is for performance testing, and I think the simpler flag is more useful for that.  Also, in its simpler form, it is a flag we will be proud of even when the behavior of @Contended is adjusted or is made part of the official spec.

The name -XX:+EnableContendedOnCP suggests it is about the -classpath, but it's not really.  I like Michael Barker's suggestion -XX:-RestrictContendedToBootClassPath, but with a default of "true".  Or, better and more simply, -XX:-RestrictContended.  This flag can be thrown away when @Contended becomes official.

So:
-XX:-EnableContended (true by default) turns off @Contended detection everywhere
-XX:-RestrictContended (true by default) allows @Contended everywhere, assuming -XX:+EnableContended
+    if (!EnableContended || (RestrictContended && !privileged))    break;

Good cleanups on the annotation collectors and field info struct.

The field layout logic gives me heartburn; it always has.  Now its 2x worse.  Would you please file a followup bug to clean up and factor that code?

On Dec 11, 2012, at 5:06 AM, Aleksey Shipilev wrote:

>> ...The rework could use something like your tagging
>> scheme (which is clever) but have the exceptional tag value mean "I
>> have extended attributes specified in N additional words of data".
> 
> I think we have a little bit of mis-design about FieldInfo here. What
> allows me to piggyback on FieldInfo is that we clearly have the
> classload-scope (CS) metadata info for fields, and runtime-scope (RS)
> metadata info. That allows us to mangle CS data during the classload,
> knowing for sure we substitute that with RS after classload finishes.
> 
> This is probably a little dodgy when compared against sparse
> representation, but I feel uncomfortable rebuilding entire field
> attribute mechanics in the course of @Contended work.

It's totally dodgy.  It makes some sense if you are aiming at an in-memory copy of the classfile AST, but is not friendly to the JVM which is actually using the data.  If I were rewriting it, I would try to use a temporary FieldInfo structure with a simple but non-dense layout, and a packing phase which would put the necessary field metadata into a dense streamable form, in an array<u1>, using CompressedStream.  The FieldStream would unpack each successive field descriptor into the FieldInfo structure.  By using CompressedStream, the data structure would be 32-bit clean, leading to various other advantages.

>> In any case, this change overlaps with the following pending change
>> from mlvm, which also introduces a field annotation: 
>> http://hg.openjdk.java.net/mlvm/mlvm/hotspot/diff/b6f0babd7cf1/anno-stable-8001107.patch
> 
>> That change makes the various method and field annotations be no-op
>> outside of privileged code. This needs to be the case also with
>> @Contended.
> 
> OK, I can do that for @Contended as well, and guard this behavior with
> two flags, e.g. -XX:+EnableContendedOnBCP=true,
> -XX:+EnableContendedOnCP=false, and act accordingly. We can discuss
> separately what should be the default values for these flags.

See above.

>> Are there SA changes and new jtreg tests?  I didn't see them in this
>> webrev.
> 
> (sigh) Any pointers what should be done, and where the tests should be
> placed?

You'll need to adjust getFieldOffset in agent/src/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java, and adjust the constant names, and add at least some of your new constants for decoding.  Start with this:

  cd $repos/hotspot; grep -ni low_offset $(hg loc -I agent)

Your unit test should be runnable with jtreg.  I think you need something like this:

 * @run main/othervm -XX:-RestrictContended Test8003985

So that this works:

  cd $repos/hotspot; jtreg test/runtime/8003985/Test8003985.java

Thanks,

— John