Re: Code Review Request For 8155757: Encapsulate impl_ methods in animation, canvas, image, input, layout, paint, and text packages
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: http://cr.openjdk.java.net/~ckyang/JDK-815575
Re: Code Review Request For 8155757: Encapsulate impl_ methods in animation, canvas, image, input, layout, paint, and text packages
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
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
9-dev unlocked following sanity testing
Re: Code Review Request For 8155757: Encapsulate impl_ methods in animation, canvas, image, input, layout, paint, and text packages
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
review for minor changes to build.gradle
Kevin, a couple of proposed changes to build.gradle. Diffs are inline in the bugs. Provide for extra jvm arguments for gradle test task https://bugs.openjdk.java.net/browse/JDK-8155831 Allow disabling :web:test https://bugs.openjdk.java.net/browse/JDK-8155832 -- David Hill Java Embedded Development "A man's feet should be planted in his country, but his eyes should survey the world." -- George Santayana (1863 - 1952)
RE: CFV: New OpenJFX Committer: Guru Hb
VOTE: YES -- Murali On 4/28/16 8:16 AM, Kevin Rushforth wrote: > I hereby nominate Guru Hb [1] to OpenJFX Committer. > > Guru is a member of JavaFX team at Oracle working on WebKit, who has > contributed 10 changesets [5] to OpenJFX, at least 8 of which are > significant. > > Votes are due by May 12, 2016. > > Only current OpenJFX Committers [2] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing > list. > > For Lazy Consensus voting instructions, see [3]. Nomination to a > project Committer is described in [4]. > > Thanks, > > -- Kevin > > [1] http://openjdk.java.net/census#ghb > > [2] http://openjdk.java.net/census#openjfx > > [3] http://openjdk.java.net/bylaws#lazy-consensus > > [4] http://openjdk.java.net/projects#project-committer > > [5] List of changesets: > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/1ec63f261e9f > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/fb4c37073893 > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/3ccf14ef836f > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/45efd3d83bf1 > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/08a57f273c76 > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/bd97f8ca31fc > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/0576d7a6f137 > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/d4f8c9496683 > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/620f5c6b4383 > http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/aa51df7e1d41 >
