RFR: 8219713: Reduce work in DefaultMethods::generate_default_methods

[Karen Kinnear]

Claes

Thank you for the j.l.Object details - that was what I expected - good to see it confirmed.

> On Mar 1, 2019, at 12:01 PM, Claes Redestad <claes.redestad at oracle.com> wrote:
> 
> 
> 
> On 2019-03-01 17:24, Karen Kinnear wrote:
>> Claes,
>> Thank you for looking at this speedup and cleanup.
>> For the j.l.Object - the logic you have changed “looks” right to me - could you possibly
>> run an example with multiple interfaces with the defaultmethods logging at debug level to show us the before and after?
> 
> For something like this:
> 
> public class A {
>    private interface I1 {
>    }
>    private interface I2 extends I1 {
>        default int foo() { return 1; }
>    }
>    private static class B implements I1, I2 {}
>    public static void main(String ... args) {
>        new B();
>    }
> }
> 
> -Xlog:defaultmethods=debug outputs this now:
> [0.093s][debug][defaultmethods] Interface A$I2 requires default method processing
> [0.093s][debug][defaultmethods] A$I2
> [0.093s][debug][defaultmethods]   java/lang/Object
> [0.093s][debug][defaultmethods]   A$I1
> [0.093s][debug][defaultmethods]     java/lang/Object
> [0.093s][debug][defaultmethods] Slots that need filling:
> [0.093s][debug][defaultmethods]   <clinit>()V
> [0.093s][debug][defaultmethods]   registerNatives()V
> [0.093s][debug][defaultmethods]   Looking for default methods for slot <clinit>()V
> [0.093s][debug][defaultmethods]   Looking for default methods for slot registerNatives()V
> [0.093s][debug][defaultmethods] Creating defaults and overpasses...
> [0.093s][debug][defaultmethods] Created 0 overpass methods
> [0.093s][debug][defaultmethods] Created 0 default  methods
> [0.093s][debug][defaultmethods] Default method processing complete
> [0.093s][debug][defaultmethods] Class A$B requires default method processing
> [0.093s][debug][defaultmethods] A$B
> [0.093s][debug][defaultmethods]   java/lang/Object
> [0.093s][debug][defaultmethods]   A$I1
> [0.093s][debug][defaultmethods]     java/lang/Object
> [0.093s][debug][defaultmethods]   A$I2
> [0.093s][debug][defaultmethods]     java/lang/Object
> [0.093s][debug][defaultmethods]     A$I1
> [0.093s][debug][defaultmethods]       java/lang/Object
> [0.093s][debug][defaultmethods] Slots that need filling:
> [0.093s][debug][defaultmethods]   foo()I
> [0.093s][debug][defaultmethods]   <clinit>()V
> [0.093s][debug][defaultmethods]   registerNatives()V
> [0.093s][debug][defaultmethods]   Looking for default methods for slot foo()I
> [0.093s][debug][defaultmethods]   Looking for default methods for slot <clinit>()V
> [0.093s][debug][defaultmethods]   Looking for default methods for slot registerNatives()V
> [0.093s][debug][defaultmethods] Creating defaults and overpasses...
> [0.093s][debug][defaultmethods] for slot: foo()I
> [0.093s][debug][defaultmethods]   Selected method: A$I2.foo()I
> [0.093s][debug][defaultmethods] Created 0 overpass methods
> [0.093s][debug][defaultmethods] Created 1 default  methods
> [0.093s][debug][defaultmethods] Default method processing complete
> 
> and with my patch:
> [0.087s][debug][defaultmethods] Interface A$I2 requires default method processing
> [0.087s][debug][defaultmethods] A$I2
> [0.087s][debug][defaultmethods]   A$I1
> [0.087s][debug][defaultmethods] Slots that need filling:
> [0.087s][debug][defaultmethods] Default method processing complete
> [0.087s][debug][defaultmethods] Class A$B requires default method processing
> [0.087s][debug][defaultmethods] A$B
> [0.087s][debug][defaultmethods]   java/lang/Object
> [0.087s][debug][defaultmethods]   A$I1
> [0.087s][debug][defaultmethods]   A$I2
> [0.087s][debug][defaultmethods]     A$I1
> [0.087s][debug][defaultmethods] Slots that need filling:
> [0.087s][debug][defaultmethods]   foo()I
> [0.087s][debug][defaultmethods]   Looking for default methods for slot foo()I
> [0.087s][debug][defaultmethods] Creating defaults and overpasses...
> [0.087s][debug][defaultmethods] for slot: foo()I
> [0.087s][debug][defaultmethods]   Selected method: A$I2.foo()I
> [0.087s][debug][defaultmethods] Created 0 overpass methods
> [0.087s][debug][defaultmethods] Created 1 default  methods
> [0.087s][debug][defaultmethods] Default method processing complete
> 
> I've manually tested numerous examples and verified the actual number of
> overpass and default methods generated doesn't change in a number of
> more complex applications.
> 
>> I am good with the reordering of the lookup vs. !already_in_vtable_slots change.
> 
> Thanks!
> 
>> I suspect the above changes (and the cleanup) are worth doing, and leave out the find_empty_vtable_slot
>> filtering changes.
> 
> I understand your reluctance with these changes. An alternative that
> comes pretty close is to specifically filter out all non-public methods
> in Object (something we explicitly do in FindMethodsByErasedSig::visit
> already):
> 
> @@ -627,14 +612,14 @@
>   while (super != NULL) {
>     for (int i = 0; i < super->methods()->length(); ++i) {
>       Method* m = super->methods()->at(i);
> -      if (m->is_overpass() || m->is_static()) {
> +      if (m->is_overpass() || (m->is_static() && !SystemDictionary::is_nonpublic_Object_method(m))) {
> ...
> 
> This keeps the property of removing examination of Object::<clinit> and
> Object::registerNatives on every type, but nothing more. Output for the
> A example above is unchanged, but private static methods and class
> initializers on all other types and interfaces would still trigger examination and behave the same as before.
Have to think about some more
> 
> 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

thanks,
Karen
> 
> Thanks!
> 
> /Claes