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

[Coleen Phillimore]

Sorry, I realized you need another reviewer so I looked more closely. 
This looks fine to me.
Coleen

On 4/29/15, 8:32 AM, Coleen Phillimore wrote:
>
> 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
>>
>