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

[Michael Haupt]

Hi Karen,

the new webrev is at http://cr.openjdk.java.net/~mhaupt/8030680/webrev.01 - all changes applied as discussed below.

Tested with:
* jtreg (java.lang.invoke, java.util.stream);
* vm.quick.testlist, vm.defmeth.testlist;
* JPRT (HotSpot testset).

Best,

Michael

> Am 28.04.2015 um 16:46 schrieb Karen Kinnear <karen.kinnear at oracle.com>:
> 
> 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