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

[Coleen Phillimore]

On 5/22/2013 7:09 PM, Aleksey Shipilev wrote:
> 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.

It's pretty hard to review one change with another bug 2 lines above 
it.   I'd rather review the cleaned up code than iterating on the not 
cleaned up code.   Especially since all the bugs are in the same 
location.   I think doing the cleanups while fixing the bugs may make it 
obvious where the next bugs are, and make it easier and less time 
consuming to review.

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

The ones that SQE wrote that are exposing these bugs.   Where are 
they?   Who wrote the SQE 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.
> 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.

You don't need a bug number for this simple change.   Usually I support 
limiting changesets but not at this level.
>
>> 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.

Not subtracting, more like not adding them:

   FieldAllocationCount fac_contended;
   FieldAllocationCount fac;
   for (AllFieldStream fs(_fields, _cp); !fs.done(); fs.next()) {
     FieldAllocationType atype = (FieldAllocationType) fs.allocation_type();
     if (fs.is_contended()) {
       fac_contended.count[atype]++;
       if (!fs.access_flags().is_static()) {
         nonstatic_contended_count++;
       }
     } else {
       fac.count[atype]++;
   }

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

I see one that I couldn't see why you were asserting it in the first place.
>> 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.

If you need directions to run these, ask me offline.

Coleen

>
> -Aleksey.
>