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

[Kevin Rushforth]

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