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

[Michael Haupt]

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?

> 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