RFR (S) CR 8014966: Add the proper Javadoc to @Contended

[Mike Duigou]

Hi Aleksey;

Прогресс! (One of the few Russian words I know thanks to the ISS)

Since the statements are grouped by fields and classes, move this to the classes paragraph:

"A contention
+ * group tag has no meaning in a class level {@code @Contended} annotation,
+ * and is ignored."


Describing the behaviour of the class annotation by referencing unavailable functionality seems strange:

"the effect is
+ * roughly equivalent to placing the {@code @Contended} annotation
+ * with the <i>same</i> anonymous tag over all the unannotated fields of
+ * the object."

The reader is left wondering "how would I annotate multiple fields with the same anonymous tag?" Perhaps a statement in the fields paragraph that "To put multiple fields into the same anonymous group use the class level @C annotation".


"over all the unannotated fields of
+ * the object."

This refers, of course, to fields without their own @C annotation not fields without annotations.


Addition: 

<p>The class level {@code @Contended} annotation is not inherited and has
+ * no effect on the fields [defined] in any sub-classes. 


Addition:

"The effects of all
+ * {@code @Contended} annotations [upon layout of object fields] remain in force for all"


As I told Brian during the lambda javadoc efforts, writing specification is the best kind of task--demanding, exacting and tedious! :-)

Mike


On May 29 2013, at 04:20 , Aleksey Shipilev wrote:

> Thanks for the review!
> 
> On 05/29/2013 04:28 AM, Martin Buchholz wrote:
>> Yeah, we have the same problem with javadoc issuing very verbose
>> warnings.  I've been ignoring/filtering them and hoping they get fixed
>> before jdk8 ships.
> 
> Ok, reverted back to <p>.
> 
>>> + * in concurrent contexts in which each instance of the annotated
>>> + * object is often accessed by a different thread.</p>
>>> 
>>> Hmmm...  makes it sound like it applies even if each instance is
>>> thread-confined (in a different thread) or is immutable.
>> 
>>    I think all the magic of ignoring @C when VM can prove the
>>    instance/fields would not experience contention, should be omitted from
>>    the documentation. The practical considerations also apply: since we can
>>    not at the moment retransform the class after the publication, it seems
>>    a good idea to treat all instances as potentially shared. This eases the
>>    reasoning (and documentation) significantly.
>> 
>>> "objects" are not annotated.
>> 
>>    Yes, should be "instances".
>> 
>> I was thinking "classes, not objects, are annotated".
> 
> Fixed.
> 
>>> however, remain in force for all
>>> + * superclass and subclass instances
>>> 
>>> 
>>> "remain in force for superclass instances" doesn't make sense to me.  Do
>>> you mean "remain in force when fields are inherited in subclasses"
>> 
>> Yes, that sounds better!
> 
> Reverted back to David's version. It does sound even cleaner in that
> version.
> 
> 
>>> Maybe "instances of the annotated class are frequently accessed by
>>> multiple threads and have fields that are frequently written".
>> 
>>    "Accessed" matters there. False sharing also happens on reads. Also, the
>>    innocent read-only classes sometimes also need the protection from the
>>    adjacent writers' racket :)
>> 
>> 
>> Hmmm... tricky ... This also then applies to pure immutable popular
>> objects like Boolean.TRUE. But you want those kinds of objects to all be
>> colocated and tightly packed with other such objects, not give each of
>> them their own cache line.
> 
> Yes, that's true. I do, however, think we have to have the provisioning
> for the read-only protected fields/instances.
> 
>> Sounds like a Quality of Implementation issue.  In general, I think you
>> *DO* want the subclass fields to be potentially in the same location as
>> the superclass padding fields.
>> A >: B
>> A layout: ppppaaaapppp
>> B layout: ppppaaaabbbbpppp
>> 
>> which is not asking too much of a jit.
>> I believe it is already common for VMs to allocate subclass fields in
>> natural padding space of superclasses.
>> 
> 
> This gets very tricky even in a slightly complicated case:
>  A >: B
>  A layout: ppppAAppppCCpppp
>  B layout: ppppAABBppCCpppp
> 
> Note that we can't move CC in B because the fields are bound statically;
> so we end up ruining the padding for both BB and CC. We can, of course,
> have the gap in the padding to tolerate this, but before knowing in
> advance what subclasses will be loaded next, we can not prepare enough;
> or, we can redefine all the classes up the hierarchy... (Current HotSpot
> classloader code is completely not ready for things like these).
> 
> However, implementation issues aside:
> 
> Allowing contended tags to be inherited will force users to look at the
> entire hierarchy to search for the colliding tags; or (what's worse)
> push the superclass maintainers to use cryptic tags so no possible
> subclasses can collide with the protected fields. IMO, that is way
> messier than pushing users to aggregate the semantically-close fields in
> the same class. Hence, I left the inheritance clause as is.
> 
> The new webrev is here:
>  http://cr.openjdk.java.net/~shade/8014966/webrev.03/
> 
> Thanks everyone, writing specs is fun!
> 
> -Aleksey.