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

[John Rose]

This is good.  I have some comments on it.

You don't need the field "bool _contended".  Use "has_annotation(_sun_misc_Contended)" instead.  That's how the other annotations work.  Having an accessor "is_contended" is great; just feed it from the "_annotations_present" bitmask.

In FieldInfo, the "type" field should be named "field_allocation_type", to avoid confusion with BasicType, etc.

It's good that you are packing the data into the field info array without adding new elements.  In the near future we are likely to add more sparse attributes to fields, which will require packing even more information into that limited space.  With that in mind, I would prefer to see us using a sparse representation sooner rather than later, but that change can wait. (In particular, the 'initval_index' is almost always zero, and should go away.)  The bottom line, for now, is that your solution for cramming in contended bits will probably need to be reworked with the next sparse field annotation (probably @Stable).  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".

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.

(As you probably noted, Doug is OK with this, with some reluctance.  The time to make such annotations more generally available may well come, but that will be when the annotation name is put into a public API.)

You can use the code from 8001107 to do this:
  http://hg.openjdk.java.net/mlvm/mlvm/hotspot/diff/b6f0babd7cf1/anno-stable-8001107.patch

The relevant hunk contains these lines:
> -ClassFileParser::AnnotationCollector::ID ClassFileParser::AnnotationCollector::annotation_index(Symbol* name) {
> +ClassFileParser::AnnotationCollector::ID
> +ClassFileParser::AnnotationCollector::annotation_index(ClassLoaderData* loader_data,
> +                                                       Symbol* name) {
>    vmSymbols::SID sid = vmSymbols::find_sid(name);
> +  bool privileged = false;
> +  if (loader_data->is_the_null_class_loader_data()) {
> +    // Privileged code can use all annotations.  Other code silently drops some.
> +    privileged = true;
> +  }
>    switch (sid) {

You will need to the class loader argument to annotation_index, etc.

I considered waiting to do this logic until we push 8001107, but there will be less trouble if you introduce it now.  Otherwise, new regression tests on @Contended will probably break when the restriction is introduced.

Are there SA changes and new jtreg tests?  I didn't see them in this webrev.

Thanks,
— John

On Dec 10, 2012, at 8:12 AM, Aleksey Shipilev wrote:

> Hi,
> 
> This is the updated webrev for JEP-142 aka @Contended:
>  http://shipilev.net/pub/jdk/hotspot/contended/webrev-6/
> 
> It employs the same trick for reducing footprint: piggybacking on
> InstanceKlass::misc_flags to store class-level flag, and making the
> tagged field in FieldInfo (as the side effect, cleaning up the mess with
> "temporarily store type into offset field") to store per-field info. I
> had to fix a few compile-time errors for MSVC.
> 
> Testing:
>  - Test8003985 @Contended-targeted test passed
>  - JPRT full cycle passed
>  - adhoc nsk, regression suites passed
> 
> I would really appreciate the reviews!
> 
> Thanks,
> Aleksey.