RFR (M): CR 8003985: Support @Contended Annotation - JEP 142

[Coleen Phillimore]

Hi,   I was going to make the same comments as Christian.

On 11/29/2012 01:56 PM, Aleksey Shipilev wrote:
> Thanks for review, Christian!
>
> On 11/29/2012 10:28 PM, Christian Thalinger wrote:
>>> The webrev for the change is here:
>>>   http://shipilev.net/pub/jdk/hotspot/contended/webrev-4/
>> Is this a review for a push?
> Yes, I'm pretty much done with functionality, it passes the tests, and
> flies through JPRT without problems.
>
>> hotspot/src/share/vm/classfile/classFileParser.cpp:
>>
>> Is it possible to factor the while-loop to align the fields into a method?
> The whole method ClassFileParser::parseClassFile cries for refactoring.
> Alas, separating the contended handling code would be more messy than
> just fitting in the same flow, since it uses quite of few locals within
> that method.

Yes, it does cry out for refactoring.  This is >900 line function now.   
I didn't see how you could refactor the new code without refactoring the 
existing code for laying out fields in a class.

The code that you added for printing looks like it could be a separate 
function though.
>
>> hotspot/src/share/vm/classfile/classFileParser.hpp:
>>
>> +    bool hasContendedAnnotation() { return has_annotation(_sun_misc_Contended); }
>>
>> We don't use camel-case method names.
> OK.
>
>> hotspot/src/share/vm/classfile/vmSymbols.hpp:
>>
>> Please keep the indent aligned.
> OK.
>
>> hotspot/src/share/vm/oops/fieldInfo.hpp:
>>
>> I wonder if we have a spare access_flags bit somewhere we could use for is_contended.
> Looking at jvm.h, I see there is only single bit left, want me to burn
> it with contended flag?

in src/share/vm/oops/klass.hpp you added the boolean _is_contended but 
it only applies to InstanceKlass, right?   It shouldn't be in all types 
of classes.   There are misc_flags in InstanceKlass that you might be 
able to use instead.   It would be a shame to burn our limited access 
flags on this thing that is unlikely.

Also, it appears that you are storing two extra u2 for each field in the 
class, where they are almost always zero.   These are saved in the 
InstanceKlass and take up a lot of space.  Jiangli's done a lot of work 
to reduce the footprint of the running vm.  One of the things she did 
was to move the generic signature indexes out of this array.   You 
should find a way to do this also.   Otherwise, you can have a side data 
structure that you save with FieldStream to allocate these fields.   You 
shouldn't add footprint for this.   We are desperately trying to save 
footprint!

Thanks,
Coleen

>
>> jdk/src/share/classes/sun/misc/Contended.java:
>>
>> Misses copyright header.
> Will update.
>
> -Aleksey.
>