Code Review Request For 8155757: Encapsulate impl_ methods in animation, canvas, image, input, layout, paint, and text packages

[Ali Ebrahimi]

Hi,
The guys at valhalla land considering mechanisms for eliminating these
garbage accessor methods under new proposal "Nestmates" for java10:

http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2016-January/000060.html

On Tue, May 3, 2016 at 2:17 AM, Kevin Rushforth <kevin.rushforth at oracle.com>
wrote:

> That sounds like a good idea. I like the idea of keeping the reduced scope
> (private where possible) for clarity if there isn't a noticeable
> performance penalty for doing so. Also, it matches our pattern for existing
> accessor methods, some of which directly access private property fields;
> changing them would be at odds with our pattern for declaring properties.
>
> -- Kevin
>
>
>
> Jim Graham wrote:
>
>> Maybe we could ask someone in Hotspot if they recognize these bytecode
>> patterns and optimize out the helper methods.  If that's the case, then it
>> is really just down to a bundled size issue...
>>
>>             ...jim
>>
>> On 5/1/2016 11:55 PM, Chien Yang wrote:
>>
>>> Hi Jim,
>>>
>>> Thanks for sharing this information and your thought. I'm not sure is
>>> this saving worth violating the principle of minimizing scope in code. I
>>> guess you did bring up a good point let me think over it and discuss
>>> with Kevin tomorrow.
>>>
>>> - Chien
>>>
>>> On 4/29/16, 4:04 PM, Jim Graham wrote:
>>>
>>>> One comment on the implementation.  For the methods used by an
>>>> accessor inner class, if you make them private in the outer class then
>>>> that inner class will need a hidden accessor method to be added in the
>>>> bytecodes.  If you make them package-private then they can access the
>>>> method directly.
>>>>
>>>> Basically, an inner class is really "just another class in the
>>>> package, but with a special name" and actually have no access to
>>>> private methods in their outer classes at all, but javac works around
>>>> this by adding a hidden method that has more open access and using that.
>>>>
>>>> An example is Image.getPlatformImage() - you turned it from "public
>>>> and impl_" into private.  Why not just leave it
>>>> package-private/default access?
>>>>
>>>> For example, compiling this class:
>>>>
>>>> public class InnerPrivateTest {
>>>>     private void foo() {}
>>>>     public class InnerClass {
>>>>         public void bar() { foo(); }
>>>>     }
>>>> }
>>>>
>>>> yields this byte code for InnerPrivateTest.class:
>>>>
>>>> public class InnerPrivateTest {
>>>>   public InnerPrivateTest();
>>>>     Code:
>>>>        0: aload_0
>>>>        1: invokespecial #2                  // Method
>>>> java/lang/Object."<init>":()V
>>>>        4: return
>>>>
>>>>   private void foo();
>>>>     Code:
>>>>        0: return
>>>>
>>>>   static void access$000(InnerPrivateTest);
>>>>     Code:
>>>>        0: aload_0
>>>>        1: invokespecial #1                  // Method foo:()V
>>>>        4: return
>>>> }
>>>>
>>>> and this for the InnerClass:
>>>>
>>>> public class InnerPrivateTest$InnerClass {
>>>>   final InnerPrivateTest this$0;
>>>>
>>>>   public InnerPrivateTest$InnerClass(InnerPrivateTest);
>>>>     Code:
>>>>        0: aload_0
>>>>        1: aload_1
>>>>        2: putfield      #1                  // Field
>>>> this$0:LInnerPrivateTest;
>>>>        5: aload_0
>>>>        6: invokespecial #2                  // Method
>>>> java/lang/Object."<init>":()V
>>>>        9: return
>>>>
>>>>   public void bar();
>>>>     Code:
>>>>        0: aload_0
>>>>        1: getfield      #1                  // Field
>>>> this$0:LInnerPrivateTest;
>>>>        4: invokestatic  #3                  // Method
>>>> InnerPrivateTest.access$000:(LInnerPrivateTest;)V
>>>>        7: return
>>>> }
>>>>
>>>> Changing the access on foo() to default (package private), yields this
>>>> byte code:
>>>>
>>>> public class InnerPackageTest {
>>>>   public InnerPackageTest();
>>>>     Code:
>>>>        0: aload_0
>>>>        1: invokespecial #1                  // Method
>>>> java/lang/Object."<init>":()V
>>>>        4: return
>>>>
>>>>   void foo();
>>>>     Code:
>>>>        0: return
>>>> }
>>>>
>>>> public class InnerPackageTest$InnerClass {
>>>>   final InnerPackageTest this$0;
>>>>
>>>>   public InnerPackageTest$InnerClass(InnerPackageTest);
>>>>     Code:
>>>>        0: aload_0
>>>>        1: aload_1
>>>>        2: putfield      #1                  // Field
>>>> this$0:LInnerPackageTest;
>>>>        5: aload_0
>>>>        6: invokespecial #2                  // Method
>>>> java/lang/Object."<init>":()V
>>>>        9: return
>>>>
>>>>   public void bar();
>>>>     Code:
>>>>        0: aload_0
>>>>        1: getfield      #1                  // Field
>>>> this$0:LInnerPackageTest;
>>>>        4: invokevirtual #3                  // Method
>>>> InnerPackageTest.foo:()V
>>>>        7: return
>>>> }
>>>>
>>>>             ...jim
>>>>
>>>> On 4/29/16 11:50 AM, Chien Yang wrote:
>>>>
>>>>> Hi Kevin,
>>>>>
>>>>> Please review the proposed fix:
>>>>>
>>>>> JIRA: https://bugs.openjdk.java.net/browse/JDK-8155757
>>>>> Webrev: http://cr.openjdk.java.net/~ckyang/JDK-8155757/webrev.00/
>>>>>
>>>>> Thanks,
>>>>> - Chien
>>>>>
>>>>


-- 

Best Regards,
Ali Ebrahimi