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

[Coleen Phillimore]

I didn't really review this but shouldn't there be a ResourceMark in the 
first TraceItables? and it would be better on more than one line with {}s.

http://cr.openjdk.java.net/~mhaupt/8030680/webrev.01/src/share/vm/oops/klassVtable.cpp.udiff.html

thanks,
Coleen

On 4/29/15, 6:41 AM, Michael Haupt wrote:
> 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
>