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

Aleksey Shipilev aleksey.shipilev at oracle.com
Tue Dec 11 05:06:54 PST 2012


Thanks for an awesome review, John! Comments inline:

On 12/11/2012 02:44 AM, John Rose wrote:
> 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.

Great, OK.

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

OK.

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

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.

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

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

-Aleksey.


More information about the hotspot-dev mailing list