RFR: 8219713: Reduce work in DefaultMethods::generate_default_methods

[Karen Kinnear]

Claes,

Lois and I met today to discuss these changes.
First - we are very grateful you dove in, identified performance issues and are proposing
solutions.

Second - could you please check in three out of the four kinds of changes:
1) remove extra walking of Object
2) remove unused code
3) change the order of the conditionals so we reduce time spent in Lookup.

And - could you please file a follow-up RFE to investigate improvements in the
default method processing for static methods, with notes about the performance benefits
of skipping processing for RegisterNatives and <clinit>?

This is important to look into. The challenge is that this needs a design proposal,
going back to the specification, history of changes and history of bugs so that we
can write down additional cases that we should add tests for, as well as agreed-upon
behaviors.

I know you have done a lot of work looking into history of changes and history of bugs
and even offered to write additional tests. Thank you for all of those - any information you
can add to the new RFE will save time and effort. 

As just one example of the detailed investigation we need, Lois pointed out for example, that we need
to also look at 8024804 - as one of several RegisterNative bugs (there are others - this is a 
painful corner case). That particular one is the reason for the filtering in FindMethodsByErasedSig:visit()
of is_nonpublic_Object_method(m) — ONLY in the case in which the class that you started
from is an interface, but not if it was a class. See JVMS Interface 5.4.3.4 Interface Method Resolution
bullet #5 - you will notice that interface method resolution differs from 5.4.3.3 Method Resolution AND
from JVMS 5.4.6 Method Selection (which never starts with an interface). The result of the default method
logic is used for all three cases. We don’t have to rathole into this example today, this is just
one example of the complexity we need to study to get this right.

Without a written investigation, those of us code reviewing this can’t be sure we are catching
all issues without essentially each doing that work ourselves. The default methods handling
is complex and you are finding some of the cases that caused us significant pain originally,
so I totally believe that there are ways to clean this up that will be more consistent and faster.
A key point that Lois made is that we want to clean up our handling of e.g. static methods in
the default method handling consistently across defaultMethods.?pp, klassVtable.?pp, 
linkResolve.?pp, instanceKlass.?pp - so all the resolution, selection and lookup logic used
by these are consistent.

Thank you for understanding that for this effort, we would like to leave the filtering logic alone
for the find_empty_vtable_slot until we do that exercise.

thanks,
Karen

> On Mar 1, 2019, at 1:55 PM, Claes Redestad <claes.redestad at oracle.com> wrote:
> 
> 
> 
> On 2019-03-01 19:34, Karen Kinnear wrote:
>>> 
>>> http://cr.openjdk.java.net/~redestad/8219713/open.02/
>>> 
>>> What do you think?
>>> 
>>> For reference, the change that added m->is_static() to the examination
>>> was: https://bugs.openjdk.java.net/browse/JDK-8028438 Lambda:
>>> default methods static superclass method masking default -
>> Thank you that is a big help.
>> The question was whether the two test cases needed for that fix were ever added to the defmeth tests since
>> they were in a separate repo at the time - my memory is they fell through the cracks:
>> Test cases added to defmeth StaticMethodsTest in review in parallel. New test cases testStaticSuperClassVsDefaultSuperInterface and testStaticLocalVsDefaultSuperInterface
> 
> Seems they came through with the VM test base open sourcing and are both
> in test/hotspot/jtreg/vmTestbase/vm/runtime/defmeth/StaticMethodsTest.java
> 
> Regardless of where we go with this I think it'd be good with versions
> of those tests that solidify the behavior w.r.t. private static method
> resolution as well.
> 
> Thanks!
> 
> /Claes
>