RFR (XS): 8030680: 292 cleanup from default method code assessment
michael.haupt at oracle.com
Mon Apr 27 15:08:03 UTC 2015
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)
> 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.
> 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
> 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.
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
More information about the hotspot-dev