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

Karen Kinnear karen.kinnear at oracle.com
Thu Apr 23 20:08:22 UTC 2015

Michael,

Thank you so much for picking this up.

So I had a couple of questions (which I should have asked when I filed the bug :-)

  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.

Not sure if we would need the local copy, i.e. nof_methods_copy - but I do appreciate that as also future proofing.

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

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

  1. Testing Thank you for the testing you already did. I realize what you changed is not extensive. I also realize that we often find a surprising interaction in changing runtime code.

So - this is what we ask folks to run when they change code in runtime, unless there are reasons to run more or less (i.e. this is the default list - since I think this is your first fix in runtime)

run vmsqe (nsk) tests: at least vm.quick.testlist run jck (noninteractive) tests: vm, lang, api run RunThese run jtreg: hotspot run jtreg: jdk run performance tests as needed - from ref workload

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 so much, Karen

p.s. I would be happy to sponsor the change - I am happy you made the changes :-)

On Apr 23, 2015, at 3:17 PM, Michael Haupt wrote:

Hello,

please review and sponsor this change, and ignore the erroneous post on core-libs-dev. RFE: https://bugs.openjdk.java.net/browse/JDK-8030680 Webrev: http://cr.openjdk.java.net/~mhaupt/8030680/webrev.00/ This is a set of simple changes to increase robustness. Tested locally with jtreg for java.lang.invoke, and with JPRT using the hotspot test set. Thanks, 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

More information about the hotspot-dev mailing list