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

[Aleksey Shipilev]

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.