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

[Karen Kinnear]

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.

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.

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.

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