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

[Aleksey Shipilev]

Thanks for review!

On 05/23/2013 02:40 AM, Coleen Phillimore wrote:
> 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? 

The idea is to have these changes separated to: a) fix the concrete
problems, and provide the targeted regression tests for them; b) provide
the basis for bisection should it bring any regressions in the future. I
don't think we should pack these up together.

> Are these contended tests being added to our hotspot/test tests?

Regression tests are, see the webrev. I'm not sure what @Contended tests
are you mentioning though.


> 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.

Ah, yes, that's clearly redundant. I filed JDK-8015254 to keep up with
the list of "ugly" issues in that code, which are not the bugs per se.
Feel free to add more insights there.

> 3555 leftover comment

To be fixed, thanks.

> 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.

Added to the cleanup list.

> 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.

That's an interesting approach, I would need to explore this a bit
more... the other use is that we can select some of the field to pack in
the gaps from the alignments of other fields, and we *don't* want those
fields to be contended. So, it would be interesting to follow if
substracting the contended fields from "fac" first time will make things
less uglier, but still correct.

> 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.

Now you're talking! :) Sure, let me run these as well.

-Aleksey.