RFR (M): CR 8003985: Support @Contended Annotation (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Thu Nov 29 11:38:31 PST 2012

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 hasannotation(sunmiscContended); } 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 accessflags bit somewhere we could use for iscontended. 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.

More information about the hotspot-dev mailing list