RFR (XS): 8030680: 292 cleanup from default method code assessment

[Karen Kinnear]

Michael,

I am good with all of your responses. Thanks for the updates and additional testing.

many thanks,
Karen

On Apr 27, 2015, at 11:08 AM, Michael Haupt wrote:

> Hi Karen,
> 
> thank you very much for your comments. My responses are below.
> 
>> Am 23.04.2015 um 22:08 schrieb Karen Kinnear <karen.kinnear at oracle.com>:
>> 1. In klassVtable::method_count_for_interface:
>> A couple of optional suggestions while you are cleaning this up:
>> 
>> If length were initialized to 0, we would not need to have two different return locations.
>> If the ASSERT was right before the return, i.e. not in the if (m->has_itable_index(),
>> we would be checking the return value for all cases and it might be more future proof
>> if someone were to extend the logic.
> 
> That's a good suggestion; thanks. I've refactored the method to have one return statement only, and moved the ASSERT block to just before that.
> 
>> Not sure if we would need the local copy, i.e. nof_methods_copy - but I do
>> appreciate that as also future proofing.
> 
> It's more defensive now - I don't think debugging code should change local state, even if that's directly before a return.
> 
>> 2. init_method_MemberName
>> I would be much more comfortable with conditional if (m.not_null() rather
>> than assert - which only does something in debug mode.
> 
> All of the checks of this sort in this method use assert() - and there are many places throughout the file (and other files) that use assert() in similar cases as well. If this should be changed to if-plus-raise-error, then that should be the case for all other similar uses of assert(), or the two that were introduced in the change are somehow special. Is this the case?
I guess the assumption is that we have generated this code internally and so this should not cause a problem. 
> 
>> 3. A favor - while you are in klassVtable.cpp ?
>> Harold and I were reading the tracing information looking at a different bug today
>> and realized some of the tracing is misleading.
>> 
>> 3A: In assign_itable_indices_for_interface it says
>> "Initializing itable for interface"
>> change to
>> "Initializing itable indices for interface"
>> 
>> If you read the comments closely, an interface does not actually have an itable, but
>> its methods need to have either a vtable index or an itable index. This code here
>> assigns itable indices to methods that do not already have a vtable index (from j.l.Object)
>> 
>> 3B:
>> assign_itable_indices_for_interface 
>>  if (m->has_vtable_index) ... "itable index %d for method ..."
>>   could you possibly change that to "vtable index %d for method"
>> 
>> that would help people be less confused debugging this.
> 
> Both applied.
> 
>> 4. Testing
>> Thank you for the testing you already did.
>> ...
>> For this particular change I would ask that in addition to what you already ran 
>> you run (with a fastdebug build):  - Christian Tornqvist can tell you how to run them
>> vm.quick.testlist
>> default methods tests (default mode and -mode invoke)
>> jtreg java/util/streams (our favorite default method shakers)
> 
> Thanks for the heads-up about testing for rt changes. I've applied the changes as described above and am now working on the testing. Will upload a revised webrev once that's successful.
> 
> Best,
> 
> Michael
> 
> -- 
> 
> <http://www.oracle.com/>
> Dr. Michael Haupt | Principal Member of Technical Staff
> Phone: +49 331 200 7277 | Fax: +49 331 200 7561
> Oracle Java Platform Group | HotSpot Compiler Team 
> Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
> <http://www.oracle.com/commitment>	Oracle is committed to developing practices and products that help protect the environment
>