RFR (S) CR 8014964: @Contended breaks has_nonstatic_fields invariant

[Coleen Phillimore]

I've been reviewing 8014886 for a few hours which is in similar code to 
this code.   The change for 8014886 appears to still have the bug that 
you have at lines 3194.   Why are these separate changes?   Can you make 
one set of changes that fixes all of the problems in these Contended 
tests?   Are these contended tests being added to our hotspot/test tests?

http://cr.openjdk.java.net/~shade/8014964/webrev.03/hotspot/src/share/vm/classfile/classFileParser.cpp.frames.html 
<http://cr.openjdk.java.net/%7Eshade/8014964/webrev.03/hotspot/src/share/vm/classfile/classFileParser.cpp.frames.html>
lines 3456 3459 contended_count == nonstatic_contended_count always.  I 
suspect the latter was distinct and not removed when static contended 
fields were removed.

3555 leftover comment

I was looking at the code under // Handle contended cases, there isn't a 
lot of variables that are passed in and out that and it could be out-lined.

On line 3146 is there a flag to test if any contended fields are present 
so that we don't rewalk the fields again?   It seems that setting 
FieldAllocationCount while parsing fields is sort of pointless if we 
have to walk the fields again.   We could just set FieldAllocationCount 
fac in this loop if we have to do this and that would eliminate the 
confusion where sometimes you subtract fac_contended->count() and 
sometimes you don't.

I couldn't figure out the new asserts with this one and am giving up for 
now.

Since I've spent the time to look at this code, I would prefer to see a 
bigger change with all the bug fixes together.   After that, please run 
the NSK tests vm.quick.testlist and run the jtreg tests in the 
hotspot/test directories, especially the compiler tests which are likely 
to have objects with different sorts of layouts and inheritance.

thanks,
Coleen

On 5/22/2013 5:16 PM, Aleksey Shipilev wrote:
> Thanks for the ideas!
>
> On 05/21/2013 08:14 PM, Vladimir Kozlov wrote:
>>> Could you move third check on next line so you can see all on one screen?
> Done.
>
>>> You are right. So parsed_annotations->is_contended() is true for any
>>> combinations of class and fields annotation. Right?
> (parsed_annotations->is_contended() == true) iff (@Contended on class).
>
> This is yet another thing to clean up.
>
>>> This could be affected by alignment even more than your assert. Could
>>> you look?
> Hm... Ok, I can compare against the non-aligned offset. It can only not
> equal the start if we do indeed have the fields.
>
> But nonstatic_field_size still has the same problem. Moreover, assert
> would seem to fail even in the simple case when the super-class is
> @Contended; added the test case for that (class R4).
>
> This is the current webrev:
>    http://cr.openjdk.java.net/~shade/8014964/webrev.03/
>
> Testing:
>    - hotspot/test/runtime/contended/ regression tests (failing)
>    - JPRT cycle vs hotspot-rt is running now  (just in case)
>
> I actually start to wonder if no size/offset is reliable for this
> assert; and counting the fields will yield the same expression over
> fac[...], which is dumb to assert. The asserts there are only to fire
> the regression test. Hence, this might seem to be that rare case where
> the regression test is impractical?
>
> -Aleksey.