Re: 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/60.html

On Tue, May 3, 2016 at 2:17 AM, Kevin Rushforth 
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."":()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."":()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."":()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."":()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: 

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

[Sergey Bylokhov]

Just FYI.
Sometimes the size is matter:
https://youtrack.jetbrains.com/issue/IDEA-74599

On 03.05.16 0:47, Kevin Rushforth 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."":()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."":()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."":()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."":()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, Sergey.

Re: 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."":()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."":()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."":()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."":()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

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

[Jim Graham]

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."":()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."":()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."":()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."":()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

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

[Chien Yang]

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."":()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."":()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."":()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."":()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

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

[Jim Graham]

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."":()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."":()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."":()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."":()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

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

[Chien Yang]

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