Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6982632 Unfortunately these changes you made back in October to lazily install the loops and leave it null have broken an internal test case (see bugid 6982632 above). This test was in the closed repository of tests, but after looking at it I think it should be moved to the open set of tests as there is nothing proprietary about it. I've attached the test case so you can take a look at it and see how you think we should fix this. The test case is making sure that we don't crash if multiple threads modify the same Graphics2D object. Note that we don't support the results of doing that, but we do have an implicit promise not to crash even if they abuse the object. With your changes we don't crash, but the test can't be run because it generates a lot of NPE exceptions whenever a rendering operation happens to catch a window where the loops are null. We can fix this either by: - Modifying the test case so that it caches exceptions and ignores them and keeps plowing on abusing the graphics to make sure that we don't crash. - Decide that perhaps exceptions are also behaviorally incompatible and find a way to modify your fix so that it never leaves the loops null and so we never get an NPE as a result. This would likely involve creating a dummy set of NOP loops and installing them whenever we would have set them to null. In either case we should push the test case into the Open sources at test/java/awt/Graphics2D/MTGraphicsAccessTest/[].java when the bug is resolved. Note that it gets a lot more bureaucratic to push fixes after the end of this week so it would be nice to get this in quickly before the bar lowers... ...jim On 10/30/2009 11:24 AM, Mario Torre wrote: Il 30/10/2009 18:50, Jennifer Godinez ha scritto: Hi Mario, Since you are pushing the fix yourself, you don't really need the "Contributed-by". This is used for someone pushing it for you. Jennifer Hi Jennifer, Jim and all, Committed! Thanks for the help, I'll celebrate with some Port wine tonight :) Cheers! Mario MTGraphicsAccessTest.java Description: java/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
2009/10/30 Mario Torre : > Il 30/10/2009 18:50, Jennifer Godinez ha scritto: >> >> Hi Mario, >> >> Since you are pushing the fix yourself, you don't really need the >> "Contributed-by". This is used for someone pushing it for you. >> >> Jennifer > > Hi Jennifer, Jim and all, > > Committed! > > Thanks for the help, I'll celebrate with some Port wine tonight :) > > Cheers! > Mario > -- > Mario Torre, Software Developer, http://www.jroller.com/neugens/ > aicas Allerton Interworks Computer Automated Systems GmbH > Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany > http://www.aicas.com * Tel: +49-721-663 968-44 > pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF > Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF > > USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim > Geschäftsführer: Dr. James J. Hunt > > Please, support open standards: > http://endsoftpatents.org/ > > w00t! http://hg.openjdk.java.net/jdk7/2d/jdk/rev/634221297c37 Congratulations! -- Andrew :-) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Support Free Java! Contribute to GNU Classpath and the OpenJDK http://www.gnu.org/software/classpath http://openjdk.java.net PGP Key: 94EFD9D8 (http://subkeys.pgp.net) Fingerprint: F8EF F1EA 401E 2E60 15FA 7927 142C 2591 94EF D9D8
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 30/10/2009 18:50, Jennifer Godinez ha scritto: Hi Mario, Since you are pushing the fix yourself, you don't really need the "Contributed-by". This is used for someone pushing it for you. Jennifer Hi Jennifer, Jim and all, Committed! Thanks for the help, I'll celebrate with some Port wine tonight :) Cheers! Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, Since you are pushing the fix yourself, you don't really need the "Contributed-by". This is used for someone pushing it for you. Jennifer Mario Torre wrote: Il 30/10/2009 15:54, Mario Torre ha scritto: Il 30/10/2009 15:53, Andrew John Hughes ha scritto: http://mail.openjdk.java.net/pipermail/build-dev/2009-October/002447.html Simple solution is to build with ALT_JDK_IMPORT_PATH set to the same as what you set ALT_BOOTDIR to for now. You shouldn't need to, but it's a quicker and simpler solution than patching the build. It should be fixed when b75 is pulled in to the 2d tree. Good Andrew, Thanks a lot, Mario Finally ready to push, this would be the Changeset entry: 6896068: SunGraphics2D exposes a reference to itself while non fully initialised. Summary: Introduce a new Interface to mark the Loops based pipes and initialise the loops accordingly. Reviewed-by: flar, rkennke Contributed-by: Mario Torre Is it OK? Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 30/10/2009 15:54, Mario Torre ha scritto: Il 30/10/2009 15:53, Andrew John Hughes ha scritto: http://mail.openjdk.java.net/pipermail/build-dev/2009-October/002447.html Simple solution is to build with ALT_JDK_IMPORT_PATH set to the same as what you set ALT_BOOTDIR to for now. You shouldn't need to, but it's a quicker and simpler solution than patching the build. It should be fixed when b75 is pulled in to the 2d tree. Good Andrew, Thanks a lot, Mario Finally ready to push, this would be the Changeset entry: 6896068: SunGraphics2D exposes a reference to itself while non fully initialised. Summary: Introduce a new Interface to mark the Loops based pipes and initialise the loops accordingly. Reviewed-by: flar, rkennke Contributed-by: Mario Torre Is it OK? Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 30/10/2009 15:53, Andrew John Hughes ha scritto: http://mail.openjdk.java.net/pipermail/build-dev/2009-October/002447.html Simple solution is to build with ALT_JDK_IMPORT_PATH set to the same as what you set ALT_BOOTDIR to for now. You shouldn't need to, but it's a quicker and simpler solution than patching the build. It should be fixed when b75 is pulled in to the 2d tree. Good Andrew, Thanks a lot, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
2009/10/30 Mario Torre : > Il 29/10/2009 13:58, Mario Torre ha scritto: >> >> Il 28/10/2009 19:43, Jennifer Godinez ha scritto: >>> >>> Hi Mario, >>> >>> Please use the sun bug ID 6896068 when pushing the change. Make sure >>> that you push it in 2D repository. If you need help, let me know. >>> >>> Thank you. >>> >>> Jennifer >> >> Hi Jennifer! >> >> I'll try to do everything today, thanks a lot for the help and support >> everybody! >> >> Cheers, >> Mario > > Hi all! > > I'm trying to get a full build up and running before the final push of this > bug. > > I got a fresh checkout of the j2d forest: > > ssh://hg.openjdk.java.net/jdk7/2d/ > > But this is what I get when I do a "LANG=C make": > > # Running javah: > /NOT-SET/re/jdk/1.7.0/promoted/latest/binaries/linux-amd64/bin/javah > -bootclasspath > /home/neugens/work_space/netbeans/openjdk-2d/build/linux-amd64/classes -d > /home/neugens/work_space/netbeans/openjdk-2d/build/linux-amd64/tmp/java/java.lang/java/CClassHeaders/ > \ > java.lang.Object java.lang.Class java.lang.Compiler > java.lang.String java.lang.Thread java.lang.ThreadGroup java.lang.StrictMath > java.lang.Number java.lang.Byte java.lang.Short java.lang.Integer > java.lang.Long java.lang.Float java.lang.Double java.lang.Boolean > java.lang.Character java.lang.System java.lang.ClassLoader java.lang.Runtime > java.lang.SecurityManager java.lang.Shutdown java.lang.Package > java.lang.ref.Finalizer java.lang.reflect.AccessibleObject > java.lang.reflect.Field java.lang.reflect.Method > java.lang.reflect.Constructor java.lang.reflect.InvocationTargetException > java.lang.reflect.Array java.lang.reflect.Proxy > java.security.AccessController java.util.Date java.util.TimeZone > java.util.ResourceBundle java.util.concurrent.atomic.AtomicLong > java.util.prefs.FileSystemPreferences java.io.Console java.io.FileDescriptor > java.io.InputStream java.io.FileInputStream java.io.FileOutputStream > java.io.PrintStream java.io.RandomAccessFile java.io.DataInputStream > java.io.DataOutputStream java.io.File java.io.FileSystem > java.io.UnixFileSystem java.io.ObjectInputStream java.io.ObjectOutputStream > java.io.ObjectStreamClass java.lang.Throwable java.lang.NoClassDefFoundError > java.lang.StringIndexOutOfBoundsException java.lang.OutOfMemoryError > sun.misc.Version sun.misc.VM sun.misc.VMSupport sun.misc.Signal > sun.misc.MessageUtils sun.misc.NativeSignalHandler sun.misc.GC > sun.reflect.ConstantPool sun.reflect.NativeConstructorAccessorImpl > sun.reflect.NativeMethodAccessorImpl sun.reflect.Reflection > java.lang.ClassLoader\$NativeLibrary > make[4]: > /NOT-SET/re/jdk/1.7.0/promoted/latest/binaries/linux-amd64/bin/javah: > Command not found > make[4]: *** > [/home/neugens/work_space/netbeans/openjdk-2d/build/linux-amd64/tmp/java/java.lang/java/obj64/.class.headers.amd64] > Error 127 > make[4]: Leaving directory > `/home/neugens/work_space/netbeans/openjdk-2d/jdk/make/java/java' > make[3]: *** [all] Error 1 > make[3]: Leaving directory > `/home/neugens/work_space/netbeans/openjdk-2d/jdk/make/java' > make[2]: *** [all] Error 1 > make[2]: Leaving directory > `/home/neugens/work_space/netbeans/openjdk-2d/jdk/make' > make[1]: *** [jdk-build] Error 2 > make[1]: Leaving directory `/home/neugens/work_space/netbeans/openjdk-2d' > make: *** [build_product_image] Error 2 > > I can do a full build of the latest jdk7 forest, so looks like something > specific of the j2d one. > > Those are the envs I use: > > export ALT_BOOTDIR=/usr/lib/jvm/java-1.6.0 > export > ALT_BINARY_PLUGS_PATH=/home/neugens/work_space/netbeans/plugs/openjdk-binary-plugs > > (I know I don't need the binary plugs). > > Looking at the build machinery looks like some envs variable are missing, > but I never had to set those explicitly, so I guess something is going wrong > in the process. > > Does anyone knows about that, or I'm doing something wrong? > > Thanks, > Mario > -- > Mario Torre, Software Developer, http://www.jroller.com/neugens/ > aicas Allerton Interworks Computer Automated Systems GmbH > Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany > http://www.aicas.com * Tel: +49-721-663 968-44 > pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF > Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF > > USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim > Geschäftsführer: Dr. James J. Hunt > > Please, support open standards: > http://endsoftpatents.org/ > > http://mail.openjdk.java.net/pipermail/build-dev/2009-October/002447.html Simple solution is to build with ALT_JDK_IMPORT_PATH set to the same as what you set ALT_BOOTDIR to for now. You shouldn't need to, but it's a quicker and simpler solution than patching the build. It should be fixed when b75 is pulled in to the 2d tree. -- Andrew :-) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Support Free Java! Contribute to GNU Classpath and the OpenJDK http://www.gnu.org/software/classpath http://openjdk.java.net PGP Key: 94EF
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 29/10/2009 13:58, Mario Torre ha scritto: Il 28/10/2009 19:43, Jennifer Godinez ha scritto: Hi Mario, Please use the sun bug ID 6896068 when pushing the change. Make sure that you push it in 2D repository. If you need help, let me know. Thank you. Jennifer Hi Jennifer! I'll try to do everything today, thanks a lot for the help and support everybody! Cheers, Mario Hi all! I'm trying to get a full build up and running before the final push of this bug. I got a fresh checkout of the j2d forest: ssh://hg.openjdk.java.net/jdk7/2d/ But this is what I get when I do a "LANG=C make": # Running javah: /NOT-SET/re/jdk/1.7.0/promoted/latest/binaries/linux-amd64/bin/javah -bootclasspath /home/neugens/work_space/netbeans/openjdk-2d/build/linux-amd64/classes -d /home/neugens/work_space/netbeans/openjdk-2d/build/linux-amd64/tmp/java/java.lang/java/CClassHeaders/ \ java.lang.Object java.lang.Class java.lang.Compiler java.lang.String java.lang.Thread java.lang.ThreadGroup java.lang.StrictMath java.lang.Number java.lang.Byte java.lang.Short java.lang.Integer java.lang.Long java.lang.Float java.lang.Double java.lang.Boolean java.lang.Character java.lang.System java.lang.ClassLoader java.lang.Runtime java.lang.SecurityManager java.lang.Shutdown java.lang.Package java.lang.ref.Finalizer java.lang.reflect.AccessibleObject java.lang.reflect.Field java.lang.reflect.Method java.lang.reflect.Constructor java.lang.reflect.InvocationTargetException java.lang.reflect.Array java.lang.reflect.Proxy java.security.AccessController java.util.Date java.util.TimeZone java.util.ResourceBundle java.util.concurrent.atomic.AtomicLong java.util.prefs.FileSystemPreferences java.io.Console java.io.FileDescriptor java.io.InputStream java.io.FileInputStream java.io.FileOutputStream java.io.PrintStream java.io.RandomAccessFile java.io.DataInputStream java.io.DataOutputStream java.io.File java.io.FileSystem java.io.UnixFileSystem java.io.ObjectInputStream java.io.ObjectOutputStream java.io.ObjectStreamClass java.lang.Throwable java.lang.NoClassDefFoundError java.lang.StringIndexOutOfBoundsException java.lang.OutOfMemoryError sun.misc.Version sun.misc.VM sun.misc.VMSupport sun.misc.Signal sun.misc.MessageUtils sun.misc.NativeSignalHandler sun.misc.GC sun.reflect.ConstantPool sun.reflect.NativeConstructorAccessorImpl sun.reflect.NativeMethodAccessorImpl sun.reflect.Reflection java.lang.ClassLoader\$NativeLibrary make[4]: /NOT-SET/re/jdk/1.7.0/promoted/latest/binaries/linux-amd64/bin/javah: Command not found make[4]: *** [/home/neugens/work_space/netbeans/openjdk-2d/build/linux-amd64/tmp/java/java.lang/java/obj64/.class.headers.amd64] Error 127 make[4]: Leaving directory `/home/neugens/work_space/netbeans/openjdk-2d/jdk/make/java/java' make[3]: *** [all] Error 1 make[3]: Leaving directory `/home/neugens/work_space/netbeans/openjdk-2d/jdk/make/java' make[2]: *** [all] Error 1 make[2]: Leaving directory `/home/neugens/work_space/netbeans/openjdk-2d/jdk/make' make[1]: *** [jdk-build] Error 2 make[1]: Leaving directory `/home/neugens/work_space/netbeans/openjdk-2d' make: *** [build_product_image] Error 2 I can do a full build of the latest jdk7 forest, so looks like something specific of the j2d one. Those are the envs I use: export ALT_BOOTDIR=/usr/lib/jvm/java-1.6.0 export ALT_BINARY_PLUGS_PATH=/home/neugens/work_space/netbeans/plugs/openjdk-binary-plugs (I know I don't need the binary plugs). Looking at the build machinery looks like some envs variable are missing, but I never had to set those explicitly, so I guess something is going wrong in the process. Does anyone knows about that, or I'm doing something wrong? Thanks, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 28/10/2009 19:43, Jennifer Godinez ha scritto: Hi Mario, Please use the sun bug ID 6896068 when pushing the change. Make sure that you push it in 2D repository. If you need help, let me know. Thank you. Jennifer Hi Jennifer! I'll try to do everything today, thanks a lot for the help and support everybody! Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, Please use the sun bug ID 6896068 when pushing the change. Make sure that you push it in 2D repository. If you need help, let me know. Thank you. Jennifer Phil Race wrote: Mario, PS .. sponsor and reviewer are different roles although quite commonly a sponsor will also be a reviewer. The sponsor is basically there to help you through the bureaucracy and the reviewer checks the actual code In this case since you have a userid you can push and I'm not sure you need a sponsor for that but Jennifer is the sponsor who will get the sun bug ID to push under. You normally need two reviewers and reviewers are supposed to be group members. At this point this fix has been reviewed well enough that I think one of the reviewers being a group member is more than good enough. In the case that its not free free to cite me .. -phil. Jennifer Godinez wrote: Hi Mario, http://bugs.openjdk.java.net/show_bug.cgi?id=100068 First, please update the above bugzilla report with the latest approved webrev. If you have signed the SCA, then go ahead and push your change. Jennifer Andrew John Hughes wrote: 2009/10/28 Mario Torre : Il 21/10/2009 23:23, Jennifer Godinez ha scritto: Hi Mario, You can submit the patch using the steps in http://openjdk.java.net/contribute/ Let me know if you have any questions. Jennifer Hi Jennifer, As far as I understand I need two sponsors, I guess one is Jim, but I'm not sure if I can count Roman as valid for the other (actually, I don't know the rules to chose the sponsors). If I understand correctly the "Looks good to go..." means either "push" or "we'll push for you", unless there is something else I have to do? Thanks, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/ Mario, Sounds to me like you can go ahead and push the patch to the 2d forest, with a Reviewed-by: flar, rkennke. You presumably have an OpenJDK account from your cacio work. Hop on IRC if you need a hand with the practicalities. Cheers,
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Mario, PS .. sponsor and reviewer are different roles although quite commonly a sponsor will also be a reviewer. The sponsor is basically there to help you through the bureaucracy and the reviewer checks the actual code In this case since you have a userid you can push and I'm not sure you need a sponsor for that but Jennifer is the sponsor who will get the sun bug ID to push under. You normally need two reviewers and reviewers are supposed to be group members. At this point this fix has been reviewed well enough that I think one of the reviewers being a group member is more than good enough. In the case that its not free free to cite me .. -phil. Jennifer Godinez wrote: Hi Mario, http://bugs.openjdk.java.net/show_bug.cgi?id=100068 First, please update the above bugzilla report with the latest approved webrev. If you have signed the SCA, then go ahead and push your change. Jennifer Andrew John Hughes wrote: 2009/10/28 Mario Torre : Il 21/10/2009 23:23, Jennifer Godinez ha scritto: Hi Mario, You can submit the patch using the steps in http://openjdk.java.net/contribute/ Let me know if you have any questions. Jennifer Hi Jennifer, As far as I understand I need two sponsors, I guess one is Jim, but I'm not sure if I can count Roman as valid for the other (actually, I don't know the rules to chose the sponsors). If I understand correctly the "Looks good to go..." means either "push" or "we'll push for you", unless there is something else I have to do? Thanks, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/ Mario, Sounds to me like you can go ahead and push the patch to the 2d forest, with a Reviewed-by: flar, rkennke. You presumably have an OpenJDK account from your cacio work. Hop on IRC if you need a hand with the practicalities. Cheers,
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, http://bugs.openjdk.java.net/show_bug.cgi?id=100068 First, please update the above bugzilla report with the latest approved webrev. If you have signed the SCA, then go ahead and push your change. Jennifer Andrew John Hughes wrote: 2009/10/28 Mario Torre : Il 21/10/2009 23:23, Jennifer Godinez ha scritto: Hi Mario, You can submit the patch using the steps in http://openjdk.java.net/contribute/ Let me know if you have any questions. Jennifer Hi Jennifer, As far as I understand I need two sponsors, I guess one is Jim, but I'm not sure if I can count Roman as valid for the other (actually, I don't know the rules to chose the sponsors). If I understand correctly the "Looks good to go..." means either "push" or "we'll push for you", unless there is something else I have to do? Thanks, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/ Mario, Sounds to me like you can go ahead and push the patch to the 2d forest, with a Reviewed-by: flar, rkennke. You presumably have an OpenJDK account from your cacio work. Hop on IRC if you need a hand with the practicalities. Cheers,
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
2009/10/28 Mario Torre : > Il 21/10/2009 23:23, Jennifer Godinez ha scritto: >> >> Hi Mario, >> >> You can submit the patch using the steps in >> http://openjdk.java.net/contribute/ >> >> Let me know if you have any questions. >> >> Jennifer > > Hi Jennifer, > > As far as I understand I need two sponsors, I guess one is Jim, but I'm not > sure if I can count Roman as valid for the other (actually, I don't know the > rules to chose the sponsors). > > If I understand correctly the "Looks good to go..." means either "push" or > "we'll push for you", unless there is something else I have to do? > > Thanks, > Mario > -- > Mario Torre, Software Developer, http://www.jroller.com/neugens/ > aicas Allerton Interworks Computer Automated Systems GmbH > Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany > http://www.aicas.com * Tel: +49-721-663 968-44 > pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF > Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF > > USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim > Geschäftsführer: Dr. James J. Hunt > > Please, support open standards: > http://endsoftpatents.org/ > > Mario, Sounds to me like you can go ahead and push the patch to the 2d forest, with a Reviewed-by: flar, rkennke. You presumably have an OpenJDK account from your cacio work. Hop on IRC if you need a hand with the practicalities. Cheers, -- Andrew :-) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Support Free Java! Contribute to GNU Classpath and the OpenJDK http://www.gnu.org/software/classpath http://openjdk.java.net PGP Key: 94EFD9D8 (http://subkeys.pgp.net) Fingerprint: F8EF F1EA 401E 2E60 15FA 7927 142C 2591 94EF D9D8
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 21/10/2009 23:23, Jennifer Godinez ha scritto: Hi Mario, You can submit the patch using the steps in http://openjdk.java.net/contribute/ Let me know if you have any questions. Jennifer Hi Jennifer, As far as I understand I need two sponsors, I guess one is Jim, but I'm not sure if I can count Roman as valid for the other (actually, I don't know the rules to chose the sponsors). If I understand correctly the "Looks good to go..." means either "push" or "we'll push for you", unless there is something else I have to do? Thanks, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, You can submit the patch using the steps in http://openjdk.java.net/contribute/ Let me know if you have any questions. Jennifer Mario Torre wrote: Il 19/10/2009 22:04, Jim Graham ha scritto: Hi Mario, That's just what I was trying to describe, thanks! Looks good to go... ...jim Hi Jim! Super cool! What's the next step now? :) Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Looks ok to me too, but I don't think I have a say.. ;-) /Roman
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 19/10/2009 22:04, Jim Graham ha scritto: Hi Mario, That's just what I was trying to describe, thanks! Looks good to go... ...jim Hi Jim! Super cool! What's the next step now? :) Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario,
That's just what I was trying to describe, thanks! Looks good to go...
...jim
Mario Torre wrote:
Il 25/09/2009 22:29, Jim Graham ha scritto:
Hi Jim,
I think we could add a quick "if (loops == null) {...}" in those 2
classes and maybe add a comment "// assert(some pipe will always be a
LoopBasedPipe)" to indicate that we aren't checking the types of the
pipes because we think it is always true in case that assumption changes
in the future. Does that make sense?
Not sure if this is what you mean:
http://cr.openjdk.java.net/~neugens/100068/webrev.09
Looks like it doesn't break stuff, I did the usual test with Java2D demo.
Cheers,
Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 25/09/2009 22:29, Jim Graham ha scritto:
Hi Jim,
I think we could add a quick "if (loops == null) {...}" in those 2
classes and maybe add a comment "// assert(some pipe will always be a
LoopBasedPipe)" to indicate that we aren't checking the types of the
pipes because we think it is always true in case that assumption changes
in the future. Does that make sense?
Not sure if this is what you mean:
http://cr.openjdk.java.net/~neugens/100068/webrev.09
Looks like it doesn't break stuff, I did the usual test with Java2D demo.
Cheers,
Mario
--
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF
USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt
Please, support open standards:
http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Sorry, after the flurry of emails here on other topics I just realized
this was getting buried.
The latest changes look good to me.
I just did a "one last grep through the sources" pass and noticed that
X11SD and GDIWindowSD also call getRenderLoops. They always call it,
though, because they install text pipes that will likely need it.
Perhaps we should check it for null before bothering with the call? I
looked quickly through and I can't totally vouch for the fact that the
loops are always needed unconditionally, but it did look likely. Also,
I think we've likely always had an extra call to validate the loops in
there so this is nothing new, but since we never were setting them to
null before we started we never had a way to check "have they been
updated by someone?" before.
I think we could add a quick "if (loops == null) {...}" in those 2
classes and maybe add a comment "// assert(some pipe will always be a
LoopBasedPipe)" to indicate that we aren't checking the types of the
pipes because we think it is always true in case that assumption changes
in the future. Does that make sense?
(Again, this isn't a new problem - just an opportunity to fix up some
old redundancy.)
...jim
Mario Torre wrote:
Il 17/09/2009 22:27, Jim Graham ha scritto:
http://cr.openjdk.java.net/~neugens/100068/webrev.08/
:)
Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 17/09/2009 22:27, Jim Graham ha scritto: http://cr.openjdk.java.net/~neugens/100068/webrev.08/ :) Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
If the NetBeans stuff is not due to changes I've made (i.e. it is due to
their overzealous "I'm going to rewrite all your build files for you
because you 'stupidly' chose to use a different version of NB than
everyone else in your project hahaha", then I just revert the files
manually (either right click and revert on the 3 files in NB, unchecking
the "save originals" checkbox), or using something like:
% hg status nbproject
{ make sure output is only the things I don't want }
% hg revert --no-backup nbproject
{ making sure there are no typos... ;-) }
As far as ignoring them in your webrev, you can do:
% hg status > webrev.files
{ edit webrev.files down to a single filename per line with "/"es }
% webrev webrev.files
Also, if you have new files in the repository then doing an "hg add"
will help include them in the webrevs, or if you use the webrev file
list technique above, then the new files will appear in the file with a
preceding "?" which you can just edit down to the name of the new file
itself and webrev will include it...
...jim
Mario Torre wrote:
Il 17/09/2009 15:22, Mario Torre ha scritto:
Il 05/08/2009 10:37, Mario Torre ha scritto:
I'll send you an update shortly based on the latest OpenJDK checkout
even.
So, "shortly" turned out to be more than one month.
I'm so sorry, I had quite few problems at work and some personal stuff
as well that kept me quite busy on other stuff. Anyway, here is an
update of the patch with the suggestions from the last comment:
http://cr.openjdk.java.net/~neugens/100068/webrev.07/
ops, of course, please ignore the netbeans stuff, I forgot to remove
them before creating the webrev (is there any way to ask webrev to
ignore some files?)
Cheers,
Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
I can't speak for the changes to the build files, but I only have a
couple of very small suggestions for the code changes.
Our coding style for continued class declarations would suggest using
the following indentation:
public class FooPipe extends SomeKindaPipe
implements LoopBasedPipe
{
with the "{" on a new line by its own after the implements clause, and
indentations of 4 spaces (see LoopPipe).
Also, the interface was not represented in the webrevs and its name
should be singular (not "Pipes")...
...jim
Mario Torre wrote:
Il 05/08/2009 10:37, Mario Torre ha scritto:
I'll send you an update shortly based on the latest OpenJDK checkout
even.
So, "shortly" turned out to be more than one month.
I'm so sorry, I had quite few problems at work and some personal stuff
as well that kept me quite busy on other stuff. Anyway, here is an
update of the patch with the suggestions from the last comment:
http://cr.openjdk.java.net/~neugens/100068/webrev.07/
I hope it's fine this time :)
Cheers,
Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 17/09/2009 15:22, Mario Torre ha scritto: Il 05/08/2009 10:37, Mario Torre ha scritto: I'll send you an update shortly based on the latest OpenJDK checkout even. So, "shortly" turned out to be more than one month. I'm so sorry, I had quite few problems at work and some personal stuff as well that kept me quite busy on other stuff. Anyway, here is an update of the patch with the suggestions from the last comment: http://cr.openjdk.java.net/~neugens/100068/webrev.07/ ops, of course, please ignore the netbeans stuff, I forgot to remove them before creating the webrev (is there any way to ask webrev to ignore some files?) Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 05/08/2009 10:37, Mario Torre ha scritto: I'll send you an update shortly based on the latest OpenJDK checkout even. So, "shortly" turned out to be more than one month. I'm so sorry, I had quite few problems at work and some personal stuff as well that kept me quite busy on other stuff. Anyway, here is an update of the patch with the suggestions from the last comment: http://cr.openjdk.java.net/~neugens/100068/webrev.07/ I hope it's fine this time :) Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 04/08/2009 23:50, Jim Graham ha scritto:
Roman Kennke wrote:
http://cr.openjdk.java.net/~neugens/100068/webrev.06/
So the short story is the webrev.05 was actually better and we better
forget about webrev.06 at this point?
It also looks like the webrev.05 is better than a stock JDK - even more
promising!
:)
Maybe someone with more experience could write a more "dramatic" test
case, but yeah, I had the feeling that they are quite similar in the
worst case, I find interesting that in 5 run each, sometimes the numbers
are quite different. I assume this is a Linux problem, it would be
interesting to run this test with a realtime VM on a realtime system :)
It's only 5x instanceof in a single place in the code and it makes the
entire business of loop validation much cleaner so I'm loving it in a
global/general sense even if it is uglier at just that one line of code.
To be honest, it would be nice to have annotation do the same as marker
interface, and being able to use them directly with instanceof without
reflection, that would make everything a bit nicer, but this is really a
minor cosmetic change, and doesn't change the ugly instanceof lines. I
passed some time on it to find a better way, but could not think of any
without the risk adding some overhead, so I gave up.
However, I would modify the code style for it to move the curly to the
following line like this:
if (foopipe instanceof blah ||
blahpipe instanceof blah ||
barpipe instanceof blah)
{
sg2d.loops = ...;
}
This is almost completely in line with our code style guidelines (are
those published on the OpenJDK site?) with the only minor variation
being the open curly on the line by itself which is a personal
preference (that is used through most of java2d) to make the break
between multi-line conditionals and body more visible. I(we?) find that
otherwise the body looks like another line of conditional tests...
Sure, that's fine! I was unsure about the coding guidelines, but this
makes entirely sense to me.
I'll send you an update shortly based on the latest OpenJDK checkout even.
Cheers,
Mario
--
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF
USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt
Please, support open standards:
http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Roman Kennke wrote:
http://cr.openjdk.java.net/~neugens/100068/webrev.06/
So the short story is the webrev.05 was actually better and we better
forget about webrev.06 at this point?
It also looks like the webrev.05 is better than a stock JDK - even more
promising!
Regarding webrev.05, I find the 5x instanceof a bit ugly. I am not sure
how to avoid it though. Maybe put the pipe fields in a container. This
container could have a (marker) subclass that is instanceof'ed for.
Whenever a SD sets up loop based pipes, it uses the subclass. Otherwise
it uses the base class. Then you'd only need one instanceof check
against the container. But OTOH you would get double field access in a
couple of cases, of which I don't know if hotspot optimizes them away
somehow. And it's probably not worth thinking about any of this if
impact is not noticable already. Maybe the if cascade bails out at the
first check already? Maybe it's worth ordering the if cascade so that
the most likely case is the first one, etc?
It's only 5x instanceof in a single place in the code and it makes the
entire business of loop validation much cleaner so I'm loving it in a
global/general sense even if it is uglier at just that one line of code.
However, I would modify the code style for it to move the curly to the
following line like this:
if (foopipe instanceof blah ||
blahpipe instanceof blah ||
barpipe instanceof blah)
{
sg2d.loops = ...;
}
This is almost completely in line with our code style guidelines (are
those published on the OpenJDK site?) with the only minor variation
being the open curly on the line by itself which is a personal
preference (that is used through most of java2d) to make the break
between multi-line conditionals and body more visible. I(we?) find that
otherwise the body looks like another line of conditional tests...
...jim
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, > I implemented the method based approach. What I did was to introduce an > interface that all loops implements, this interface consist of a single > method that returns a boolean. > > I made the implementation final everywhere to try to limit the impact of > it, although I'm not sure this is 100% correct in all cases, but the > impact is still too high. In my opinion we should try to go with the > interface approach, it's really less costly and less complex, basically > no impact at all in all the tests. .. > Finally, the webrew: > > http://cr.openjdk.java.net/~neugens/100068/webrev.06/ So the short story is the webrev.05 was actually better and we better forget about webrev.06 at this point? Regarding webrev.05, I find the 5x instanceof a bit ugly. I am not sure how to avoid it though. Maybe put the pipe fields in a container. This container could have a (marker) subclass that is instanceof'ed for. Whenever a SD sets up loop based pipes, it uses the subclass. Otherwise it uses the base class. Then you'd only need one instanceof check against the container. But OTOH you would get double field access in a couple of cases, of which I don't know if hotspot optimizes them away somehow. And it's probably not worth thinking about any of this if impact is not noticable already. Maybe the if cascade bails out at the first check already? Maybe it's worth ordering the if cascade so that the most likely case is the first one, etc? > Please, ignore the ".hgignore" changes in the patch. Haha. ;-) Cheers, Roman
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 22/07/2009 02:11, Jim Graham ha scritto: It's odd that J2DBench showed a difference since nothing you did should have affected those benchmarks. I don't think it has any benchmark which shows the impact of a pipeline validation, so your standalone test is the only one that I think addresses the issue. Rather than setting up a machine for a 72 hour run (not sure a run of what, though - J2DBench?), I'd rather see either trying to do the check with a virtual method call (Pipe.needsLoops) or just going back to the old style of initializing them in the validation branches that we know need them and we can revisit this mechanism some other time when we have the time to really figure out how to make it cheap. In particular, I have some ideas for how to make validation incredibly cheap at the cost of a few K of lookup tables per SurfaceData, but I think the scale of that is beyond us for now... ...jim Hi Jim, I tried some more of the j2d benchmark but you're right, looks like no one of them causes a revalidate, so they are all useless for this specific case. I implemented the method based approach. What I did was to introduce an interface that all loops implements, this interface consist of a single method that returns a boolean. I made the implementation final everywhere to try to limit the impact of it, although I'm not sure this is 100% correct in all cases, but the impact is still too high. In my opinion we should try to go with the interface approach, it's really less costly and less complex, basically no impact at all in all the tests. I attach two results here. The first run iterates 100 times, two times for warm-up, then one more time to do the actual measurement. The second run is more "longish" :) each loop is augmented to 1. As I make this test and collect the stats via a script (well, attached, but it's not the coolest script in the world), I didn't make it smart enough to tell you what each JDK actually is, so this is the explanation: OPEN_JDK0 == build 1.7.0-internal-neugens_2009_07_27_18_10-b00 contains the patch in webrew.06 (the one with the methods, linked here) OPEN_JDK1 == build 1.7.0-internal-neugens_2009_07_15_15_31-b00 is a pure JDK build OPEN_JDK2 == build 1.7.0-internal-neugens_2009_07_15_14_30-b00 is the "old" webrew.05 patch (marker interface). Finally, the webrew: http://cr.openjdk.java.net/~neugens/100068/webrev.06/ Please, ignore the ".hgignore" changes in the patch. Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/ start tests ---===*** ../openjdk/build/linux-amd64/bin/java: ***===--- openjdk version "1.7.0-internal" OpenJDK Runtime Environment (build 1.7.0-internal-neugens_2009_07_27_18_10-b00) OpenJDK 64-Bit Server VM (build 16.0-b05, mixed mode) ../openjdk/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main 2>&1 | tee -a OPEN_JDK0.results warmed up run time in ms: 524 total time in ms: 1782 ../openjdk/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main 2>&1 | tee -a OPEN_JDK0.results warmed up run time in ms: 316 total time in ms: 1366 ../openjdk/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main 2>&1 | tee -a OPEN_JDK0.results warmed up run time in ms: 360 total time in ms: 1394 ../openjdk/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main 2>&1 | tee -a OPEN_JDK0.results warmed up run time in ms: 444 total time in ms: 1505 ../openjdk/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main 2>&1 | tee -a OPEN_JDK0.results warmed up run time in ms: 644 total time in ms: 1751 ---===*** end: ../openjdk/build/linux-amd64/bin/java ***===--- ---===*** ../openjdk-proxy/build/linux-amd64/bin/java: ***===--- openjdk version "1.7.0-internal" OpenJDK Runtime Environment (build 1.7.0-internal-neugens_2009_07_15_15_31-b00) OpenJDK 64-Bit Server VM (build 16.0-b05, mixed mode) ../openjdk-proxy/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main 2>&1 | tee -a OPEN_JDK1.results warmed up run time in ms: 327 total time in ms: 1028 ../openjdk-proxy/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main 2>&1 | tee -a OPEN_JDK1.results warmed up run time in ms: 331 total time in ms: 1041 ../openjdk-proxy/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main 2>&1 | tee -a OPEN_JDK1.results warmed up run time in ms: 323 total time in ms: 1053 ../openjdk-proxy/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main 2>&1 | tee -a OPEN_JDK1.results
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
It's odd that J2DBench showed a difference since nothing you did should have affected those benchmarks. I don't think it has any benchmark which shows the impact of a pipeline validation, so your standalone test is the only one that I think addresses the issue. Rather than setting up a machine for a 72 hour run (not sure a run of what, though - J2DBench?), I'd rather see either trying to do the check with a virtual method call (Pipe.needsLoops) or just going back to the old style of initializing them in the validation branches that we know need them and we can revisit this mechanism some other time when we have the time to really figure out how to make it cheap. In particular, I have some ideas for how to make validation incredibly cheap at the cost of a few K of lookup tables per SurfaceData, but I think the scale of that is beyond us for now... ...jim Mario Torre wrote: Il 15/07/2009 23:41, Jim Graham ha scritto: Numbers that small aren't statistically significant. Our J2DBench benchmark calibrates each test to run a number of iterations that result in at least 2.5 seconds of run time. Try upping your loop iterations by a factor of 100 and you'll get numbers with better accuracy and precision... ...jim Hi Jim, I multiplied the small test numbers by 100 and this is the result: Patched JDK: warmed up run time in ms: 3226 total time in ms: 9586 Clean JDK: warmed up run time in ms: 3039 total time in ms: 9172 I also run the more meaningful Java2DBench. I had no time to run an extensive benchmark, and only limited to what I think it's the very bare minimum: http://cr.openjdk.java.net/~neugens/100068/comparision-100068-0.1/Summary_Report.html So, looks like this approach has indeed an impact, although is well within 10%. Should I try the other approaches? I hardly see how a method call can be faster than an instanceof, but maybe I miss something obvious. If you want, I can try to setup a machine at work to run a full 72 hour test, but this will take some time ( > 72*2 hours :). Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 15/07/2009 23:41, Jim Graham ha scritto: Numbers that small aren't statistically significant. Our J2DBench benchmark calibrates each test to run a number of iterations that result in at least 2.5 seconds of run time. Try upping your loop iterations by a factor of 100 and you'll get numbers with better accuracy and precision... ...jim Hi Jim, I multiplied the small test numbers by 100 and this is the result: Patched JDK: warmed up run time in ms: 3226 total time in ms: 9586 Clean JDK: warmed up run time in ms: 3039 total time in ms: 9172 I also run the more meaningful Java2DBench. I had no time to run an extensive benchmark, and only limited to what I think it's the very bare minimum: http://cr.openjdk.java.net/~neugens/100068/comparision-100068-0.1/Summary_Report.html So, looks like this approach has indeed an impact, although is well within 10%. Should I try the other approaches? I hardly see how a method call can be faster than an instanceof, but maybe I miss something obvious. If you want, I can try to setup a machine at work to run a full 72 hour test, but this will take some time ( > 72*2 hours :). Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Numbers that small aren't statistically significant. Our J2DBench benchmark calibrates each test to run a number of iterations that result in at least 2.5 seconds of run time. Try upping your loop iterations by a factor of 100 and you'll get numbers with better accuracy and precision... ...jim Mario Torre wrote: [neug...@galactica OpenJDKPerformaceTest]$ ../openjdk/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main warmed up run time in ms: 32 total time in ms: 144 [neug...@galactica OpenJDKPerformaceTest]$ ../openjdk-proxy/build/linux-amd64/bin/java -cp build/classes/ net.java.openjdk.Main warmed up run time in ms: 31 total time in ms: 157
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 14/07/2009 21:07, Jim Graham ha scritto:
I would set the AA hint or set the composite - one or the other - but
not both. Basically you are looking for the minimum action that causes
an invalidation/revalidation cycle in order to measure the impact. The
more extraneous work in the inner loop, the less impact you will notice
from any overhead the new code might have added.
In both cases I was using debug build (as make debug_build) on an
Intel Core2 Duo T8300 running at full speed (2.40GHz).
I would also compare optimized to optimized as the debug build probably
adds a bit of overhead on its own - again, which could mask any overhead
your changes might have added.
Maybe I should run a more scientific test, but this doesn't look too
bad to me :)
True - it's a nice quick check result, but to be sure it should be done
with minimal test overhead on an optimized build...
...jim
Yes boss :)
[neug...@galactica OpenJDKPerformaceTest]$
../openjdk/build/linux-amd64/bin/java -cp build/classes/
net.java.openjdk.Main
warmed up run time in ms: 32
total time in ms: 144
[neug...@galactica OpenJDKPerformaceTest]$
../openjdk-proxy/build/linux-amd64/bin/java -cp build/classes/
net.java.openjdk.Main
warmed up run time in ms: 31
total time in ms: 157
I did various run of this guy, the results are similar, the range of the
total time goes from 144 to 158 basically for both OpenJDK version, but
the warmed up time is always around 30-32 ms for both, so I don't see
any performance degradation.
Btw, the non debug build is *fast*, I never realised this because I'm
too used to run debug enabled code, that hotspot does magics is known,
but this difference is quite amazing...
I attached the test case but, of course, is really simple. I'm going to
do a test run with some real app too, like NetBeans :). The Java2D demo
isn't really the fastest thing on Earth with any *JDK version I tried,
so it may not be the best use case to spot for performance issues.
Cheers,
Mario
--
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF
USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt
Please, support open standards:
http://endsoftpatents.org/
package net.java.openjdk;
import java.awt.AlphaComposite;
import java.awt.Graphics2D;
import java.awt.RenderingHints;
import java.awt.image.BufferedImage;
import javax.swing.JFrame;
/**
* Very simple microbenchmark to test performance impact in changes
* caused by OpenJDK bug #100068.
*
* @author Mario Torre
*/
public class Main {
/**
* @param args the command line arguments
*/
public static void main(String[] args) {
BufferedImage bimg =
new BufferedImage(100, 100, BufferedImage.TRANSLUCENT);
Graphics2D g = bimg.createGraphics();
long _start = System.currentTimeMillis();
for (int i = 0; i < 10; i++) {
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_OFF);
g.fillRect(0,0,1,1);
}
for (int i = 0; i < 10; i++) {
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_OFF);
g.fillRect(0,0,1,1);
}
long start = System.currentTimeMillis();
for (int i = 0; i < 10; i++) {
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_OFF);
g.fillRect(0,0,1,1);
}
long stop = System.currentTimeMillis();
System.out.println("warmed up run time in ms: " + (stop - start));
System.out.println("total time in ms: " + (stop - _start));
}
}
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario,
...jim
Mario Torre wrote:
I tried to figure out some "common" use case, so i did this (based on
your suggestion too), I found this combination to force the pipe to be
invalidated:
BufferedImage bimg =
new BufferedImage(100, 100, BufferedImage.TRANSLUCENT);
Graphics2D g = bimg.createGraphics();
for (int i = 0; i < 10; i++) {
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
g.setComposite(AlphaComposite.getInstance(
AlphaComposite.SRC_OVER, 0.5f));
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_OFF);
g.setComposite(AlphaComposite.getInstance(
AlphaComposite.SRC_OVER, 1.0f));
g.fillRect(0,0,1,1);
}
I would set the AA hint or set the composite - one or the other - but
not both. Basically you are looking for the minimum action that causes
an invalidation/revalidation cycle in order to measure the impact. The
more extraneous work in the inner loop, the less impact you will notice
from any overhead the new code might have added.
In both cases I was using debug build (as make debug_build) on an Intel
Core2 Duo T8300 running at full speed (2.40GHz).
I would also compare optimized to optimized as the debug build probably
adds a bit of overhead on its own - again, which could mask any overhead
your changes might have added.
Maybe I should run a more scientific test, but this doesn't look too bad
to me :)
True - it's a nice quick check result, but to be sure it should be done
with minimal test overhead on an optimized build...
...jim
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 09/07/2009 22:23, Jim Graham ha scritto:
- pipe interfaces implement a new interface "LoopBasedPipes" which has
no methods, it is just a "marker" interface. The test in validate() just
uses instanceof in this case which might be faster than an interface
method dispatch (actually, it almost certainly would be).
Hi Jim,
http://cr.openjdk.java.net/~neugens/100068/webrev.05/
I did it with the interface. Actually, I could not spot any real
difference, but maybe I'm doing a micro benchmark.
I tried to figure out some "common" use case, so i did this (based on
your suggestion too), I found this combination to force the pipe to be
invalidated:
BufferedImage bimg =
new BufferedImage(100, 100, BufferedImage.TRANSLUCENT);
Graphics2D g = bimg.createGraphics();
for (int i = 0; i < 10; i++) {
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
g.setComposite(AlphaComposite.getInstance(
AlphaComposite.SRC_OVER, 0.5f));
g.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_OFF);
g.setComposite(AlphaComposite.getInstance(
AlphaComposite.SRC_OVER, 1.0f));
g.fillRect(0,0,1,1);
}
I do this loop 3 times, so to make hotspot warm up first.
After some runs, I get this as an average (well, manually rounded to be
honest):
warmed up run time in ms: 614
total time in ms: 2189
With "plain" OpenJDK I get similar results.
In both cases I was using debug build (as make debug_build) on an Intel
Core2 Duo T8300 running at full speed (2.40GHz).
I used the "almost latest" OpenJDK code drop, with changeset ID
a50217eb3ee1.
Maybe I should run a more scientific test, but this doesn't look too bad
to me :)
Cheers,
Mario
--
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF
USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt
Please, support open standards:
http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, Mario Torre wrote: So here is the deal. I'm going back to the original plan to fix the issue, this time without accessors. I'll just invalidate the loops and revalidate them elsewhere. The question that still remains is when to re-validate the loops? My idea is to make the pipes that use the loops to tell this themselves, so we cover future problems if we're going to refactor this stuff more. I'm going to work on this a bit this week, of course, if you already planned some direction, just tell me (maybe I wait your reply this time :) And I'll try to give it in a timely fashion. ;-) I think it would be nice to have the pipes manage the need for the loops, but it could be touchy to try to work this into the logic in some of the spaghetti validate methods. (I'm not recommending against it, just warning you to keep an eye out for if you start rat-holing trying to get that to work.) A couple of ways I can see this working (in order of my estimation of performance impact, low to high): - pipe interfaces implement a new interface "LoopBasedPipes" which has no methods, it is just a "marker" interface. The test in validate() just uses instanceof in this case which might be faster than an interface method dispatch (actually, it almost certainly would be). - pipe interfaces add "public boolean needsLoops()" which requires an interface method call per pipe. - pipe interfaces add "public void setup(SG2D)" and they add the loops if they are null (loops are set to null on invalidate), but this would require lots of "if null" checks and the implementation would be pretty much the same everywhere. Validate would have to call the setup on every pipe before it returns. To test if you've impacted performance I don't think we have any test cases that measure this particular code path. I would use a test case that does something cheap to induce a revalidate, like: setRH(AA_ON) // No need to render, just use it to invalidate setRH(AA_OFF) // Now that the AA is set back to the fast mode, // render something tiny fillRect(1x1) in a tight loop. If there is a cheaper attribute to tweak to cause invalidations that would be better - I think the AA hint is trapped and just sets a field internally, but maybe setting a Composite rule back and forth might be even cheaper. Simply changing the color would not tend to cause an invalidate (though changing it from solid to translucent might, but the pixel value would have to be revalidated as well). ...jim
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 09/07/2009 02:40, Jim Graham ha scritto: Hi Jim! Even more to the point, the sheer size of this patch is disruptive. I don't know if there is much of a chance of a cut/paste error in all of it - perhaps you used an automated tool to make the changes and so the textual replacement is somewhat reliable, but at the very least there is now a bit of busy-work on the other end for someone to review the patch and verify that every "mumblex" and "mumbley" reference got changed into the corresponding "getMumbleX()" and "getMumbleY()" accessors. Yeah, I see, I tried to be as careful as possible, and manually checked all the references (and did a full compile after each field was refactored!) and this was already painful. A reviewer must take even more care and I supposed this patch was just too much code. In the end, I am very sorry that I didn't bring all of this up earlier in more detail. I was time-pressured and wrote just a basic summary of where I thought it would make sense to spend time on this bug fix and now I feel that the appropriate response here is to say that I'd vote against this particular change, unfortunately and very much not happily after you've done all this work. :-( No problem, I've learned lots of stuff about the internals of Java2D that way anyway. So here is the deal. I'm going back to the original plan to fix the issue, this time without accessors. I'll just invalidate the loops and revalidate them elsewhere. The question that still remains is when to re-validate the loops? My idea is to make the pipes that use the loops to tell this themselves, so we cover future problems if we're going to refactor this stuff more. I'm going to work on this a bit this week, of course, if you already planned some direction, just tell me (maybe I wait your reply this time :) I am open to hearing how having accessors here might improve our development at this point, but my gut feel after having worked on this code for almost a decade is that we are better off without them. Am I missing something? And if you want to vent some frustration - fire away (privately hopefully)... No worries, I'm going to kill myself, but don't feel too guilty (just a bit ;) Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, I'm sorry that the communication path here is so thin. I would have loved to have discussed this particular effort in more detail before you got into it very far. Unfortunately, the end of June was a busy time for me and I didn't review and respond to your message on 6/24 as I should have. I hate to dampen enthusiasm here, but if I had to prioritize the tasks you mentioned then I might have preferred one of the other tasks you had outlined first - mainly because, as I get to below, I might have recommended that you not do this part. The fields have been working just fine so far as they are and this patch fixes no bugs in the code. If we were instituting new policies for a given field that required an accessor then I could buy into adding one, but the changes here are not mandated by any policy changes under consideration. And with respect to code cleanliness, private and public fields are not a big deal in internal code that doesn't have to promise backwards compatibility. When we need to change a field, we do so and nothing breaks because there are no external dependencies - we own all of the code that needs to be updated. Even more to the point, the sheer size of this patch is disruptive. I don't know if there is much of a chance of a cut/paste error in all of it - perhaps you used an automated tool to make the changes and so the textual replacement is somewhat reliable, but at the very least there is now a bit of busy-work on the other end for someone to review the patch and verify that every "mumblex" and "mumbley" reference got changed into the corresponding "getMumbleX()" and "getMumbleY()" accessors. I also actually would prefer to keep as many bare field references in the code as possible for other reasons. There is nothing that paints a better picture as to the possible performance ramifications of a particular attribute fetch as a bare field reference. If I am writing performance critical code then I know exactly how expensive "sg2d.pixel" is going to be, but I have to wonder and investigate if "sg2d.getPixel()" is going to be enough of a problem to have to cache the value locally or not. If everything looks like a method call, it becomes that much harder and more time consuming to write fast code and verify if any of the many patches that we review are going to affect performance or not. To that end, I rather see the accessors as a problem rather than a solution. This is more of an issue in performance-intensive code like the internals of a graphics engine than in application code. In the end, I am very sorry that I didn't bring all of this up earlier in more detail. I was time-pressured and wrote just a basic summary of where I thought it would make sense to spend time on this bug fix and now I feel that the appropriate response here is to say that I'd vote against this particular change, unfortunately and very much not happily after you've done all this work. :-( I am open to hearing how having accessors here might improve our development at this point, but my gut feel after having worked on this code for almost a decade is that we are better off without them. Am I missing something? And if you want to vent some frustration - fire away (privately hopefully)... ...jim Mario Torre wrote: Il 02/07/2009 17:07, Roman Kennke ha scritto: Hi Mario, Would you mind if I divide the patch in 2 or 3 slots? Actually this is a great idea! After completing each slot, you could check with J2DBench if performance is actually affected or not. I'll try to give you something already today, hopefully. Yay! I'm still waiting ;-) Ugh, that one sucked time :) Ok, so here is the webrew for the "make all fields private" part: http://cr.openjdk.java.net/~neugens/100068/webrev.04/ Things to say: 1. Looks like some fields are never accessed outside SunGraphics2D, no accessor are provided for those. 2. AffineTransform was a lot of fun to make correctly, because we need both the "raw" transform and the the transform returned by getTransform, because in some areas we need to know about clipping and device coordinates, while in some other places we don't want to know about that. I added a getRawTransform, I hope this name is meaningfull. 3. I tested carefully, but I may have missed something, so I need some help with this, it's a huge patch. I need help to test the windows fluff especially. 4. Everything is made final, but in some places we may still cache stuff returned by the getters, I'll do this if this patch is ok and gets committed. 5. Just because I don't like to have 4 points :) If this is ok, I'll move on the other things. Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 08/07/2009 13:54, Mario Torre ha scritto: 5. Just because I don't like to have 4 points :) If this is ok, I'll move on the other things. Ah, actually I found a real 5 :) I want to document the accessors, but I didn't because the fields are not documented. Most of them are obvious, but I'll add documentation at some later point. Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 02/07/2009 17:07, Roman Kennke ha scritto: Hi Mario, Would you mind if I divide the patch in 2 or 3 slots? Actually this is a great idea! After completing each slot, you could check with J2DBench if performance is actually affected or not. I'll try to give you something already today, hopefully. Yay! I'm still waiting ;-) Ugh, that one sucked time :) Ok, so here is the webrew for the "make all fields private" part: http://cr.openjdk.java.net/~neugens/100068/webrev.04/ Things to say: 1. Looks like some fields are never accessed outside SunGraphics2D, no accessor are provided for those. 2. AffineTransform was a lot of fun to make correctly, because we need both the "raw" transform and the the transform returned by getTransform, because in some areas we need to know about clipping and device coordinates, while in some other places we don't want to know about that. I added a getRawTransform, I hope this name is meaningfull. 3. I tested carefully, but I may have missed something, so I need some help with this, it's a huge patch. I need help to test the windows fluff especially. 4. Everything is made final, but in some places we may still cache stuff returned by the getters, I'll do this if this patch is ok and gets committed. 5. Just because I don't like to have 4 points :) If this is ok, I'll move on the other things. Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, Would you mind if I divide the patch in 2 or 3 slots? Actually this is a great idea! After completing each slot, you could check with J2DBench if performance is actually affected or not. I'll try to give you something already today, hopefully. Yay! I'm still waiting ;-) Cheers, Roman
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 23/06/2009 01:47, Jim Graham ha scritto: Hi Mario, The changes look safe, but I wanted to bring up the following issues: Hi Jim, - There are a dozen or so fields in SG2D that are commonly accessed everywhere in the pipelines including loops. These changes introduce an accessor for loops, but that now looks out of place with no accessors for any of the other fields that get accessed directly. Personally the lack of accessors hasn't bothered me, particularly since this is performance sensitive code and accessors can sap performance by nickels and dimes if you don't implement them correctly - to wit: +42 :) - Accessors can be inlined if they are final, but these aren't. As it turns out, SG2D itself is final and so I believe that is enough for them to be inlined, but I tend to make methods final as well to underscore that they are intended to be inlined, and also in case we eventually decide to make the class non-final. On the other hand, we haven't really bothered with accessors in the first place in this code to avoid the code bloat and the potential points of heuristic failure. - I agree that it would be nice to ask the pipes if they need loops, I'm not sure why your solution didn't work, but I prefer that to making them a side effect of getTextPipe() since it makes it harder to know when the code you are editing needs to get the loops or not, and it makes it hard to see where they come from when editing the many validatePipe() methods. Would you mind if I divide the patch in 2 or 3 slots? I'll give you first the accessors patch, then a patch that fixes the issue in subject with my current approach and finally a refactorisation so that the loops can ask wherever they want the loops validated or not, because this is the less trivial and because I cannot work on that much in my company time :(. I'll try to give you something already today, hopefully. Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi, > - Accessors can be inlined if they are final, but these aren't. As it turns > out, SG2D itself is final and so I believe that is enough for them to be > inlined, but I tend to make methods final as well to underscore that they > are intended to be inlined, and also in case we eventually decide to make > the class non-final. The client-compiler can inline virtual methods since 1.4, however it inserts a conditional to trap if the target is not of the expected type - so setting stuff final at least can't hurt ;) I really hope tiered compiulation will make it into Java7. - Clemens
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, The changes look safe, but I wanted to bring up the following issues: - There are a dozen or so fields in SG2D that are commonly accessed everywhere in the pipelines including loops. These changes introduce an accessor for loops, but that now looks out of place with no accessors for any of the other fields that get accessed directly. Personally the lack of accessors hasn't bothered me, particularly since this is performance sensitive code and accessors can sap performance by nickels and dimes if you don't implement them correctly - to wit: - Accessors can be inlined if they are final, but these aren't. As it turns out, SG2D itself is final and so I believe that is enough for them to be inlined, but I tend to make methods final as well to underscore that they are intended to be inlined, and also in case we eventually decide to make the class non-final. On the other hand, we haven't really bothered with accessors in the first place in this code to avoid the code bloat and the potential points of heuristic failure. - I agree that it would be nice to ask the pipes if they need loops, I'm not sure why your solution didn't work, but I prefer that to making them a side effect of getTextPipe() since it makes it harder to know when the code you are editing needs to get the loops or not, and it makes it hard to see where they come from when editing the many validatePipe() methods. ...jim Mario Torre wrote: Il 18/06/2009 21:52, Jim Graham ha scritto: One solution would be to always set the loops for the validations that install one of these pipes. That could have potential performance impact, but it would be no worse than the validation sequences that already set the loops every time so I don't think it would be serious, or even measurable. Hi Jim, I'm trying this approach. Basically, I just set the loops to null now in invalidate and validate them back in validatePipe: http://cr.openjdk.java.net/~neugens/100068/webrev.03/ What I do is to get valid Loops when we call getTextPipe, but other than that, the behavior is basically unchanged. I tried with various applications, including NetBeans, the Java2D demo and the FontTest app, playing around with the text configurations (AA, LCD, size, Glyphs etc.. even output to a PNG file) and looks good, but I may miss something of course. I moved to a 64 bit machine, I don't think this makes any difference in this case, but maybe it's a good thing to say. I would like to see a method in the pipelines that does something like: boolean mustInitialiseLoopsBecauseWeReallyWantToUseThemGranted() or so, that we may call at the end of the validatePipe method, but when I tried this I got a funny StackOverflowException, maybe I did something wrong, but looks like it's not so easy to change this code and still make it behave correctly, so I would like to go deeper in this issue even if you think we can close the bug (of course if the proposed fix is evaluated as complete). Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il 18/06/2009 21:52, Jim Graham ha scritto: One solution would be to always set the loops for the validations that install one of these pipes. That could have potential performance impact, but it would be no worse than the validation sequences that already set the loops every time so I don't think it would be serious, or even measurable. Hi Jim, I'm trying this approach. Basically, I just set the loops to null now in invalidate and validate them back in validatePipe: http://cr.openjdk.java.net/~neugens/100068/webrev.03/ What I do is to get valid Loops when we call getTextPipe, but other than that, the behavior is basically unchanged. I tried with various applications, including NetBeans, the Java2D demo and the FontTest app, playing around with the text configurations (AA, LCD, size, Glyphs etc.. even output to a PNG file) and looks good, but I may miss something of course. I moved to a 64 bit machine, I don't think this makes any difference in this case, but maybe it's a good thing to say. I would like to see a method in the pipelines that does something like: boolean mustInitialiseLoopsBecauseWeReallyWantToUseThemGranted() or so, that we may call at the end of the validatePipe method, but when I tried this I got a funny StackOverflowException, maybe I did something wrong, but looks like it's not so easy to change this code and still make it behave correctly, so I would like to go deeper in this issue even if you think we can close the bug (of course if the proposed fix is evaluated as complete). Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Whoops! I abbreviated and sent you off on a wild goose chase. There are many different loops! There just happen to be (I think!?) only 1 kind of *TEXT* loop in the system for a given surface. I left out that qualifier below and misdirected you. Thus, if the text loops were split out then it "may be" that they can be created once per SurfaceData, but that is definitely *NOT* true of the other loops that reside in the RenderLoops, and it may not be true of the text loops in the future, it just happens to be "probably" true for text loops for now... ...jim Roman Kennke wrote: Hi Jim, If only one instance of the loops is used during the lifetime of a Surface, then we can easily keep this instance there instead of the SG2D, validating would not initialize any new objects, but only put stuff together that is already present in the SurfaceData anyway. Or did I get something wrong here? i / Roman Jim Graham wrote: That makes a lot of sense since I think the introduction of the GlyphListLoopPipe in its current form was where the original methodology of "always install loops if you plan to use loops" was first (and only time) violated. I think I raised the issue at the time and Phil pointed out that, in practice, the loops never really change for a given surface type (as in, if a different compositing mode is set or if a non-color paint is set then we use entirely different mechanisms to render so the only loops that exist for these are "solid color" loops and those loops are always valid for all cases that use these pipes). While that may mean we don't have a practical bug yet, the weakness of that argument in general, and the desire to resolve the issue from the original bug report may mean we should revisit this practice. One solution would be to always set the loops for the validations that install one of these pipes. That could have potential performance impact, but it would be no worse than the validation sequences that already set the loops every time so I don't think it would be serious, or even measurable. Another solution could involve splitting these loops out into a separate structure that can be set once in the lifetime of an SG2D and then reused as appropriate. But this is banking on the "only one version of these loops really exists anyway" constraint and some day that constraint is likely to disappear and then we really will need to pick new loops on every validate. As such, I think I'd prefer the first solution - to just pick new loops every time they are needed (i.e. unless the pipeline really doesn't use the loops directly, or indirectly through these text pipes)... ...jim Mario Torre wrote: Il giorno gio, 18/06/2009 alle 00.34 +0200, Mario Torre ha scritto: But I'll debafterug it a little further and send you some updates asap. Cheers, Mario Hello! The pipelines that use a loops after being invalidated without checking if it's valid or not (null) are those so far: LCDTextRenderer GlyphListLoopPipe AATextRenderer Those all are subclasses of GlyphListLoopPipe. The LCDTextRenderer never fails if I don't explicitly set to null the loops in invalidatePipe, same happens for instances of GlyphListLoopPipe which don't fail immediately as when I set the loops to null, so I think some of those calls use loops there were initialised somehow from validatePipe in some previous call to this method, but that should not be valid anymore. There are no other places where I get NPE, but I've tested only with the Java2D demo so far. Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno gio, 18/06/2009 alle 17.36 -0400, Roman Kennke ha scritto: > Hi folks, > > The thing is, it might be the case that we only have one loops thing in > the pipelines we have in OpenJDK, but thanks to Cacio (hohum) we must > consider that there are other implementations out there... which might > want to take completely different approaches. So better to keep it clean > and nice and not make any assumptions. Right? > > / Roman Well, if we specify the behaviour should be ok. Actually I'm not sure about the best way to go, at this point validate/invalidate may make more sense. Have to think a bit more on that... Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi folks, The thing is, it might be the case that we only have one loops thing in the pipelines we have in OpenJDK, but thanks to Cacio (hohum) we must consider that there are other implementations out there... which might want to take completely different approaches. So better to keep it clean and nice and not make any assumptions. Right? / Roman Mario Torre wrote: Il giorno gio, 18/06/2009 alle 16.01 -0400, Roman Kennke ha scritto: Hi Jim, If only one instance of the loops is used during the lifetime of a Surface, then we can easily keep this instance there instead of the SG2D, validating would not initialize any new objects, but only put stuff together that is already present in the SurfaceData anyway. Or did I get something wrong here? i This make sense, because we can initialise the loops in one place and live with them. If things will change, there will be no risk to forget because the whole thing is in one single place now and has no extra dependencies anymore, but from what I see the loops are always the same from a practical point of view (but I'll check this a bit better just in case). I like both this and the first approach proposed by Jim, so is up to you to decide what to do :) Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno gio, 18/06/2009 alle 16.01 -0400, Roman Kennke ha scritto: > Hi Jim, > > If only one instance of the loops is used during the lifetime of a > Surface, then we can easily keep this instance there instead of the > SG2D, validating would not initialize any new objects, but only put > stuff together that is already present in the SurfaceData anyway. Or did > I get something wrong here? > i This make sense, because we can initialise the loops in one place and live with them. If things will change, there will be no risk to forget because the whole thing is in one single place now and has no extra dependencies anymore, but from what I see the loops are always the same from a practical point of view (but I'll check this a bit better just in case). I like both this and the first approach proposed by Jim, so is up to you to decide what to do :) Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/ -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Jim, If only one instance of the loops is used during the lifetime of a Surface, then we can easily keep this instance there instead of the SG2D, validating would not initialize any new objects, but only put stuff together that is already present in the SurfaceData anyway. Or did I get something wrong here? i / Roman Jim Graham wrote: That makes a lot of sense since I think the introduction of the GlyphListLoopPipe in its current form was where the original methodology of "always install loops if you plan to use loops" was first (and only time) violated. I think I raised the issue at the time and Phil pointed out that, in practice, the loops never really change for a given surface type (as in, if a different compositing mode is set or if a non-color paint is set then we use entirely different mechanisms to render so the only loops that exist for these are "solid color" loops and those loops are always valid for all cases that use these pipes). While that may mean we don't have a practical bug yet, the weakness of that argument in general, and the desire to resolve the issue from the original bug report may mean we should revisit this practice. One solution would be to always set the loops for the validations that install one of these pipes. That could have potential performance impact, but it would be no worse than the validation sequences that already set the loops every time so I don't think it would be serious, or even measurable. Another solution could involve splitting these loops out into a separate structure that can be set once in the lifetime of an SG2D and then reused as appropriate. But this is banking on the "only one version of these loops really exists anyway" constraint and some day that constraint is likely to disappear and then we really will need to pick new loops on every validate. As such, I think I'd prefer the first solution - to just pick new loops every time they are needed (i.e. unless the pipeline really doesn't use the loops directly, or indirectly through these text pipes)... ...jim Mario Torre wrote: Il giorno gio, 18/06/2009 alle 00.34 +0200, Mario Torre ha scritto: But I'll debafterug it a little further and send you some updates asap. Cheers, Mario Hello! The pipelines that use a loops after being invalidated without checking if it's valid or not (null) are those so far: LCDTextRenderer GlyphListLoopPipe AATextRenderer Those all are subclasses of GlyphListLoopPipe. The LCDTextRenderer never fails if I don't explicitly set to null the loops in invalidatePipe, same happens for instances of GlyphListLoopPipe which don't fail immediately as when I set the loops to null, so I think some of those calls use loops there were initialised somehow from validatePipe in some previous call to this method, but that should not be valid anymore. There are no other places where I get NPE, but I've tested only with the Java2D demo so far. Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
That makes a lot of sense since I think the introduction of the GlyphListLoopPipe in its current form was where the original methodology of "always install loops if you plan to use loops" was first (and only time) violated. I think I raised the issue at the time and Phil pointed out that, in practice, the loops never really change for a given surface type (as in, if a different compositing mode is set or if a non-color paint is set then we use entirely different mechanisms to render so the only loops that exist for these are "solid color" loops and those loops are always valid for all cases that use these pipes). While that may mean we don't have a practical bug yet, the weakness of that argument in general, and the desire to resolve the issue from the original bug report may mean we should revisit this practice. One solution would be to always set the loops for the validations that install one of these pipes. That could have potential performance impact, but it would be no worse than the validation sequences that already set the loops every time so I don't think it would be serious, or even measurable. Another solution could involve splitting these loops out into a separate structure that can be set once in the lifetime of an SG2D and then reused as appropriate. But this is banking on the "only one version of these loops really exists anyway" constraint and some day that constraint is likely to disappear and then we really will need to pick new loops on every validate. As such, I think I'd prefer the first solution - to just pick new loops every time they are needed (i.e. unless the pipeline really doesn't use the loops directly, or indirectly through these text pipes)... ...jim Mario Torre wrote: Il giorno gio, 18/06/2009 alle 00.34 +0200, Mario Torre ha scritto: But I'll debafterug it a little further and send you some updates asap. Cheers, Mario Hello! The pipelines that use a loops after being invalidated without checking if it's valid or not (null) are those so far: LCDTextRenderer GlyphListLoopPipe AATextRenderer Those all are subclasses of GlyphListLoopPipe. The LCDTextRenderer never fails if I don't explicitly set to null the loops in invalidatePipe, same happens for instances of GlyphListLoopPipe which don't fail immediately as when I set the loops to null, so I think some of those calls use loops there were initialised somehow from validatePipe in some previous call to this method, but that should not be valid anymore. There are no other places where I get NPE, but I've tested only with the Java2D demo so far. Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno gio, 18/06/2009 alle 00.34 +0200, Mario Torre ha scritto: > But I'll debafterug it a little further and send you some updates asap. > > Cheers, > Mario Hello! The pipelines that use a loops after being invalidated without checking if it's valid or not (null) are those so far: LCDTextRenderer GlyphListLoopPipe AATextRenderer Those all are subclasses of GlyphListLoopPipe. The LCDTextRenderer never fails if I don't explicitly set to null the loops in invalidatePipe, same happens for instances of GlyphListLoopPipe which don't fail immediately as when I set the loops to null, so I think some of those calls use loops there were initialised somehow from validatePipe in some previous call to this method, but that should not be valid anymore. There are no other places where I get NPE, but I've tested only with the Java2D demo so far. Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi everybody, I go back to my previous comment that perhaps what we need is for invalidatePipe to set the loops to null and then fix all the places where we were relying on "old loops" by setting them intentionally in the corresponding validate sequences... I am completely in favour of that approach, even if it costs more time (after all, it's Mario's time in the end ;-) ). Relying on some more or less random old loop hanging around is asking for bugs, the subtle kind, those that eat up all your time when the next deadline is lurking around the corner... ;-) / Roman ...jim Phil Race wrote: I don't think it is related to the LCD text work. I think it was the JDK 7 b17 fix: 6263951: Java does not use fast AA text loops when regular AA hint is set. So there's a requirement that renderLoops is non-null in some case when they would not previously have been used. -phil. Jim Graham wrote: Ah, I think I see the problem. Phil can chime in here if I'm wrong. Text now uses loops in most cases, but many of the validate methods assume that they don't need the loops. I don't think that used to be the case, but it changed as a result of the LCD text loop work. It used to be that AA used the alphafill field of SG2D and not the loops, and it still does for most rendering. But now text rendering will almost always go through the loops and it is only working because of that call to getRenderLoops() in the constructor (and the fact that we never set it back to null. I'd like to see invalidate() set the loops to null and see how far we get - I'll bet that we'd get NPEs all over the place, especially with AA rendering. In the long run, this is another straw on the camel's back as far as rethinking the validation framework. It's been tweaked and hacked quite a lot over the past 10 years and so there are a lot of issues that arise like this that aren't readily obvious from the code. I don't think the camel's back is broken yet, but it is becoming more and more confusing for new engineers to start tinkering with it without seeing things break from seemingly innocuous changes. :-( For now, I'm wary of removing that call without a lot of testing. I think putting a "loops=null" line in invalidatePipe() might accelerate some of that testing, though. And Phil might want to chime in with a description of the new requirements on loops in light of the post-LCD text work... Phil? ...jim
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno mer, 17/06/2009 alle 14.07 -0700, Jim Graham ha scritto: > I go back to my previous comment that perhaps what we need is for > invalidatePipe to set the loops to null and then fix all the places > where we were relying on "old loops" by setting them intentionally in > the corresponding validate sequences... > I think the exceptions attached in my last mail are the two most obvious places to start, because the other pipes install a new loops via validatePipe(). But I'll debug it a little further and send you some updates asap. Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
I guess the thing that bothers me here is that the cases which "incidentally" rather than "intentionally" rely on the text loops may be using whatever text loops happened to have been installed by a prior validation pipeline, because they clearly are not installing any. If those pipelines can use a text loop then they need to "intentionally" install the right text loop rather than rely on whatever is already installed, otherwise the wrong rendering attributes (composite mode or paint type, etc.) may be used, no? So, doesn't this represent a (potential) bug which won't be fixed even if we do lazy initialization of the loops? I go back to my previous comment that perhaps what we need is for invalidatePipe to set the loops to null and then fix all the places where we were relying on "old loops" by setting them intentionally in the corresponding validate sequences... ...jim Phil Race wrote: I don't think it is related to the LCD text work. I think it was the JDK 7 b17 fix: 6263951: Java does not use fast AA text loops when regular AA hint is set. So there's a requirement that renderLoops is non-null in some case when they would not previously have been used. -phil. Jim Graham wrote: Ah, I think I see the problem. Phil can chime in here if I'm wrong. Text now uses loops in most cases, but many of the validate methods assume that they don't need the loops. I don't think that used to be the case, but it changed as a result of the LCD text loop work. It used to be that AA used the alphafill field of SG2D and not the loops, and it still does for most rendering. But now text rendering will almost always go through the loops and it is only working because of that call to getRenderLoops() in the constructor (and the fact that we never set it back to null. I'd like to see invalidate() set the loops to null and see how far we get - I'll bet that we'd get NPEs all over the place, especially with AA rendering. In the long run, this is another straw on the camel's back as far as rethinking the validation framework. It's been tweaked and hacked quite a lot over the past 10 years and so there are a lot of issues that arise like this that aren't readily obvious from the code. I don't think the camel's back is broken yet, but it is becoming more and more confusing for new engineers to start tinkering with it without seeing things break from seemingly innocuous changes. :-( For now, I'm wary of removing that call without a lot of testing. I think putting a "loops=null" line in invalidatePipe() might accelerate some of that testing, though. And Phil might want to chime in with a description of the new requirements on loops in light of the post-LCD text work... Phil? ...jim
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
I don't think it is related to the LCD text work. I think it was the JDK 7 b17
fix:
6263951: Java does not use fast AA text loops when regular AA hint is set.
So there's a requirement that renderLoops is non-null in some case when
they would not previously have been used.
-phil.
Jim Graham wrote:
Ah, I think I see the problem. Phil can chime in here if I'm wrong.
Text now uses loops in most cases, but many of the validate methods
assume that they don't need the loops. I don't think that used to be
the case, but it changed as a result of the LCD text loop work. It used
to be that AA used the alphafill field of SG2D and not the loops, and it
still does for most rendering. But now text rendering will almost
always go through the loops and it is only working because of that call
to getRenderLoops() in the constructor (and the fact that we never set
it back to null.
I'd like to see invalidate() set the loops to null and see how far we
get - I'll bet that we'd get NPEs all over the place, especially with AA
rendering.
In the long run, this is another straw on the camel's back as far as
rethinking the validation framework. It's been tweaked and hacked quite
a lot over the past 10 years and so there are a lot of issues that arise
like this that aren't readily obvious from the code. I don't think the
camel's back is broken yet, but it is becoming more and more confusing
for new engineers to start tinkering with it without seeing things break
from seemingly innocuous changes. :-(
For now, I'm wary of removing that call without a lot of testing. I
think putting a "loops=null" line in invalidatePipe() might accelerate
some of that testing, though.
And Phil might want to chime in with a description of the new
requirements on loops in light of the post-LCD text work... Phil?
...jim
Mario Torre wrote:
Il giorno lun, 15/06/2009 alle 13.37 -0700, Jim Graham ha scritto:
Hi Mario,
How are the drawGlyphList methods called when the loops is null? I
ask because they are only ever installed on the SunGraphics2D object
by virtue of a call to validatePipe() and all calls to validatePipe()
should set the loops. So, there is a bug somewhere that causes these
loops to be installed without a full validate process.
Hi Jim,
So, I spent some time today (sorry for the delay, I had to find some
free time slot for that and had to make a cake for my girl friend, which
was the most difficult part :), and I debugged the Java2D demo with just
the non initialised loops (so, no checks for null loops anywhere else in
the code).
The Demo starts fine, but after some point I get the error attached in
this mail.
As I said in the email you quoted. All calls from pipelines
(GlyphListLoopPipe and AATextRenderer are both pipeline objects)
should be safe because all calls to validatePipe() should set the loops.
I see that validatePipe is indeed called always, but sometimes the path
that is chosen doesn't validate the rendering loop. I've seen that
most of the time this is ok, because the loops are not used.
I guess this is the case for all the various text rendering (LCD or AA)
in swing components, for example (is this correct?).
Having said that I note that there are some pipelines that do not
require loops and it would be OK for a call to validate that only
uses such "non-loop-based" pipelines to leave the loops field
uninitialized, but all validate calls which use loop-based pipes must
update the loops field.
Exactly. I'm not deep into the code yet to distinguish when they are
needed or not, but...
Eliminating all of those uses of loops we then fall into the case
where the usage in checkFontInfo is the only usage that does not
occur from a pipeline...
...this is exactly the case, putting a null check here solves the NPE.
For what I can see, at some point some component needs to paint to an
offscreen surface (I don't think the offscreen surface is special,
I think it's the application code that drives the bug, but anyway...).
This is the background image from the Java2D Intro pane, I think the
other pane do something similar. Once the SunGraphics2d object is
created,
some redering hints are set. This is the application code:
g2.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
That is, turn on antialiasing, then:
g2.clearRect(0, 0, d.width, d.height);
Now what? This of course goes through validatePipe:
The first two if/else statements fall through but not this
(SurfaceData, line 535 in OpenJDK):
} else if (sg2d.antialiasHint == SunHints.INTVAL_ANTIALIAS_ON) {
This guy sets the pipes but doesn't set the RederingLoops.
Then, again application code:
scene.render(d.width, d.height, g2);
Which after few loops finally renders the string on screen,
which causes the crash.
So, in other words, everything goes fine until we draw text with
the AA redering hints turned on.
Of course, before it was not faili
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno mar, 16/06/2009 alle 16.18 -0700, Jim Graham ha scritto: > Ah, I think I see the problem. Phil can chime in here if I'm wrong. Yay, glad it helped :) > Text now uses loops in most cases, but many of the validate methods > assume that they don't need the loops. I don't think that used to be > the case, but it changed as a result of the LCD text loop work. It used > to be that AA used the alphafill field of SG2D and not the loops, and it > still does for most rendering. But now text rendering will almost > always go through the loops and it is only working because of that call > to getRenderLoops() in the constructor (and the fact that we never set > it back to null. That makes completely sense to me. > I'd like to see invalidate() set the loops to null and see how far we > get - I'll bet that we'd get NPEs all over the place, especially with AA > rendering. I'll try that tomorrow. I can also help with the testing, I would like to see those changes in the main code base at some point, and they will be surely useful for Cacio too. > In the long run, this is another straw on the camel's back as far as > rethinking the validation framework. It's been tweaked and hacked quite > a lot over the past 10 years and so there are a lot of issues that arise > like this that aren't readily obvious from the code. I don't think the > camel's back is broken yet, but it is becoming more and more confusing > for new engineers to start tinkering with it without seeing things break > from seemingly innocuous changes. :-( Eh... I've seen this in the FontManager code too, and in Classpath we had similar problems for some of the most obscure things too, that's an obvious consequence of code that has to preserve backward forever-compatibility, but I have to say that the Java2D code does some really cool things in many places :) > And Phil might want to chime in with a description of the new > requirements on loops in light of the post-LCD text work... Phil? That would be interesting :) Cheers (and good night!), Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/ -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Ah, I think I see the problem. Phil can chime in here if I'm wrong.
Text now uses loops in most cases, but many of the validate methods
assume that they don't need the loops. I don't think that used to be
the case, but it changed as a result of the LCD text loop work. It used
to be that AA used the alphafill field of SG2D and not the loops, and it
still does for most rendering. But now text rendering will almost
always go through the loops and it is only working because of that call
to getRenderLoops() in the constructor (and the fact that we never set
it back to null.
I'd like to see invalidate() set the loops to null and see how far we
get - I'll bet that we'd get NPEs all over the place, especially with AA
rendering.
In the long run, this is another straw on the camel's back as far as
rethinking the validation framework. It's been tweaked and hacked quite
a lot over the past 10 years and so there are a lot of issues that arise
like this that aren't readily obvious from the code. I don't think the
camel's back is broken yet, but it is becoming more and more confusing
for new engineers to start tinkering with it without seeing things break
from seemingly innocuous changes. :-(
For now, I'm wary of removing that call without a lot of testing. I
think putting a "loops=null" line in invalidatePipe() might accelerate
some of that testing, though.
And Phil might want to chime in with a description of the new
requirements on loops in light of the post-LCD text work... Phil?
...jim
Mario Torre wrote:
Il giorno lun, 15/06/2009 alle 13.37 -0700, Jim Graham ha scritto:
Hi Mario,
How are the drawGlyphList methods called when the loops is null? I ask
because they are only ever installed on the SunGraphics2D object by
virtue of a call to validatePipe() and all calls to validatePipe()
should set the loops. So, there is a bug somewhere that causes these
loops to be installed without a full validate process.
Hi Jim,
So, I spent some time today (sorry for the delay, I had to find some
free time slot for that and had to make a cake for my girl friend, which
was the most difficult part :), and I debugged the Java2D demo with just
the non initialised loops (so, no checks for null loops anywhere else in
the code).
The Demo starts fine, but after some point I get the error attached in
this mail.
As I said in the email you quoted. All calls from pipelines
(GlyphListLoopPipe and AATextRenderer are both pipeline objects) should
be safe because all calls to validatePipe() should set the loops.
I see that validatePipe is indeed called always, but sometimes the path
that is chosen doesn't validate the rendering loop. I've seen that
most of the time this is ok, because the loops are not used.
I guess this is the case for all the various text rendering (LCD or AA)
in swing components, for example (is this correct?).
Having said that I note that there are some pipelines that do not
require loops and it would be OK for a call to validate that only uses
such "non-loop-based" pipelines to leave the loops field uninitialized,
but all validate calls which use loop-based pipes must update the loops
field.
Exactly. I'm not deep into the code yet to distinguish when they are
needed or not, but...
Eliminating all of those uses of loops we then fall into the case where
the usage in checkFontInfo is the only usage that does not occur from a
pipeline...
...this is exactly the case, putting a null check here solves the NPE.
For what I can see, at some point some component needs to paint to an
offscreen surface (I don't think the offscreen surface is special,
I think it's the application code that drives the bug, but anyway...).
This is the background image from the Java2D Intro pane, I think the
other pane do something similar. Once the SunGraphics2d object is created,
some redering hints are set. This is the application code:
g2.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
That is, turn on antialiasing, then:
g2.clearRect(0, 0, d.width, d.height);
Now what? This of course goes through validatePipe:
The first two if/else statements fall through but not this
(SurfaceData, line 535 in OpenJDK):
} else if (sg2d.antialiasHint == SunHints.INTVAL_ANTIALIAS_ON) {
This guy sets the pipes but doesn't set the RederingLoops.
Then, again application code:
scene.render(d.width, d.height, g2);
Which after few loops finally renders the string on screen,
which causes the crash.
So, in other words, everything goes fine until we draw text with
the AA redering hints turned on.
Of course, before it was not failing because of the rendering loops
were initialised in the constructor.
I'm not sure if we need to check for a null loops at the beginning
of validatePipe or in checkFontoInfo. Maybe we can save something if we
use checkFontInfo but I'm not so sure.
I hope this helps,
Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno lun, 15/06/2009 alle 13.37 -0700, Jim Graham ha scritto:
> Hi Mario,
>
> How are the drawGlyphList methods called when the loops is null? I ask
> because they are only ever installed on the SunGraphics2D object by
> virtue of a call to validatePipe() and all calls to validatePipe()
> should set the loops. So, there is a bug somewhere that causes these
> loops to be installed without a full validate process.
Hi Jim,
So, I spent some time today (sorry for the delay, I had to find some
free time slot for that and had to make a cake for my girl friend, which
was the most difficult part :), and I debugged the Java2D demo with just
the non initialised loops (so, no checks for null loops anywhere else in
the code).
The Demo starts fine, but after some point I get the error attached in
this mail.
> As I said in the email you quoted. All calls from pipelines
> (GlyphListLoopPipe and AATextRenderer are both pipeline objects) should
> be safe because all calls to validatePipe() should set the loops.
I see that validatePipe is indeed called always, but sometimes the path
that is chosen doesn't validate the rendering loop. I've seen that
most of the time this is ok, because the loops are not used.
I guess this is the case for all the various text rendering (LCD or AA)
in swing components, for example (is this correct?).
> Having said that I note that there are some pipelines that do not
> require loops and it would be OK for a call to validate that only uses
> such "non-loop-based" pipelines to leave the loops field uninitialized,
> but all validate calls which use loop-based pipes must update the loops
> field.
Exactly. I'm not deep into the code yet to distinguish when they are
needed or not, but...
> Eliminating all of those uses of loops we then fall into the case where
> the usage in checkFontInfo is the only usage that does not occur from a
> pipeline...
...this is exactly the case, putting a null check here solves the NPE.
For what I can see, at some point some component needs to paint to an
offscreen surface (I don't think the offscreen surface is special,
I think it's the application code that drives the bug, but anyway...).
This is the background image from the Java2D Intro pane, I think the
other pane do something similar. Once the SunGraphics2d object is created,
some redering hints are set. This is the application code:
g2.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
RenderingHints.VALUE_ANTIALIAS_ON);
That is, turn on antialiasing, then:
g2.clearRect(0, 0, d.width, d.height);
Now what? This of course goes through validatePipe:
The first two if/else statements fall through but not this
(SurfaceData, line 535 in OpenJDK):
} else if (sg2d.antialiasHint == SunHints.INTVAL_ANTIALIAS_ON) {
This guy sets the pipes but doesn't set the RederingLoops.
Then, again application code:
scene.render(d.width, d.height, g2);
Which after few loops finally renders the string on screen,
which causes the crash.
So, in other words, everything goes fine until we draw text with
the AA redering hints turned on.
Of course, before it was not failing because of the rendering loops
were initialised in the constructor.
I'm not sure if we need to check for a null loops at the beginning
of validatePipe or in checkFontoInfo. Maybe we can save something if we
use checkFontInfo but I'm not so sure.
I hope this helps,
Mario
--
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF
USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt
Please, support open standards:
http://endsoftpatents.org/
--
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF
USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt
Please, support open standards:
http://endsoftpatents.org/
In various place after some time the demo is started or when selecting the
ArcsCurves panel:
In Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
at sun.java2d.pipe.AATextRenderer.drawGlyphList(AATextRenderer.java:40)
at sun.java2d.pipe.GlyphListPipe.drawString(GlyphListPipe.java:72)
at sun.java2d.SunGraphics2D.drawString(SunGraphics2D.java:2808)
at demos.Arcs_Curves.Arcs.drawDemo(Arcs.java:128)
at DemoSurface.paint(DemoSurface.java:303)
at javax.swing.JComponent.paintToOffscreen(JCompone
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Not necessarily. The demo serves to test a wide variety of 2D functionality all in one place. It has worked very well to point out real bugs in the past that we have fixed, though it does have some questionable practices inside it that can turn up false bugs and I believe its abuse of threading is one such practice. Send me (off-line) the full stack traces of the failures you were seeing and I'll check and see if they occur in areas where it might be trying to do graphics from multiple threads at once... ...jim Mario Torre wrote: Il giorno mar, 16/06/2009 alle 12.08 -0700, Jim Graham ha scritto: It's probably also a good place to let slip that a certain demo that was mentioned previously in the thread has always been considered "poorly written" and I wouldn't put it past it to be violating the threading design goals we have. But I would never come right out and say something like that since it serves as a good "make sure even kooky things continue to work" telltale... [goes back to looking nonchalant] Ugh... don't tell me I'm debugging with a wrong test case... :S Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno mar, 16/06/2009 alle 12.08 -0700, Jim Graham ha scritto: > It's probably also a good place to let slip that a certain demo that was > mentioned previously in the thread has always been considered "poorly > written" and I wouldn't put it past it to be violating the threading > design goals we have. But I would never come right out and say > something like that since it serves as a good "make sure even kooky > things continue to work" telltale... [goes back to looking nonchalant] > Ugh... don't tell me I'm debugging with a wrong test case... :S Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
I should clarify that what I meant by "doesn't corrupt anything" is that the world won't come to an end or the system crash. It's protected against harm from MT, but it doesn't support MT. And another way of stating it is that we won't consider race condition bugs when it is used in an MT fashion, but we would consider fixing bugs that cause it to crash or that cause a G2D to latch onto a specific thread. It's probably also a good place to let slip that a certain demo that was mentioned previously in the thread has always been considered "poorly written" and I wouldn't put it past it to be violating the threading design goals we have. But I would never come right out and say something like that since it serves as a good "make sure even kooky things continue to work" telltale... [goes back to looking nonchalant] ...jim Jim Graham wrote: My design goal was "doesn't corrupt anything if run from multiple threads" (in other words don't require synchronization in any common places just to keep it functional and don't require it to be used from a particular thread), but only correct behavior if run from 1 thread at a time. In other words, it can be used by multiple threads in sequence, but not simultaneously. ...jim
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
My design goal was "doesn't corrupt anything if run from multiple threads" (in other words don't require synchronization in any common places just to keep it functional and don't require it to be used from a particular thread), but only correct behavior if run from 1 thread at a time. In other words, it can be used by multiple threads in sequence, but not simultaneously. ...jim Roman Kennke wrote: Hi there, first of all, let me say that - especially in the light of this thread, I support Mario's change (removal of this one line from the constructor) for the following reasons: - It should not be necessary as you say, this field should always be initialized before use by validatePipe(). - If it's not, it's most likely a bug. - Therefore this line is only good for hiding bugs. One thing is not quite clear to me: - A race condition where thread A is validating the pipeline and installs the pipeline objects but hasn't yet reached the code to install the loops while thread B starts rendering using that SG2D thus invoking an operation on a partially initialized pipeline - in this case the NPE is appropriate and allowed since we don't support multi-threaded use of the Graphics2D objects. I was always under the assumption, that Java2D should be thread safe. And in many places we make sure it is, e.g. in the SurfaceData objects. But now you say, it isn't. So what is the real thing about Java2D and thread safety. Is it only supposed to be thread safe when each thread gets its own Graphics2D object? I think I remember back in the GNU Classpath days we had a test case that shared a Graphics (no Graphics2D back then..) object between threads and was supposed to be ok, but I might be wrong here... Can you enlighten me what is the situation w/ Java2D and thread safety? / Roman
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi there, first of all, let me say that - especially in the light of this thread, I support Mario's change (removal of this one line from the constructor) for the following reasons: - It should not be necessary as you say, this field should always be initialized before use by validatePipe(). - If it's not, it's most likely a bug. - Therefore this line is only good for hiding bugs. One thing is not quite clear to me: - A race condition where thread A is validating the pipeline and installs the pipeline objects but hasn't yet reached the code to install the loops while thread B starts rendering using that SG2D thus invoking an operation on a partially initialized pipeline - in this case the NPE is appropriate and allowed since we don't support multi-threaded use of the Graphics2D objects. I was always under the assumption, that Java2D should be thread safe. And in many places we make sure it is, e.g. in the SurfaceData objects. But now you say, it isn't. So what is the real thing about Java2D and thread safety. Is it only supposed to be thread safe when each thread gets its own Graphics2D object? I think I remember back in the GNU Classpath days we had a test case that shared a Graphics (no Graphics2D back then..) object between threads and was supposed to be ok, but I might be wrong here... Can you enlighten me what is the situation w/ Java2D and thread safety? / Roman
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario,
Ah, I can see that I was behind a bit in my emails.
The question I still have is why the loops can be null while valid
pipelines are installed?
The SG2D starts life with invalid pipes installed, which means that
*ALL* graphics operations get vectored through a validate process before
any work is done. Since no pipeline operation objects are installed at
that point (i.e. all pipeline fields in SG2D have a reference to an
InvalidPipe instance installed in them), then no references to loops can
occur. When the InvalidPipe methods are invoked it validates the SG2D,
which both installs real pipelines and fills in the loops member, and
then reinvokes on the new pipelines - which should be OK since the loops
field should have been installed by then. So, some other bug must
exist, one of:
- Some validatePipe implementation in some SurfaceData class is
installing a loop-based pipeline without initializing the loops
- Some code is calling a method on a loop-based pipeline that it did not
get from the SG2D itself (i.e. it got it from a different SG2D, but
invoked it on this SG2D - or it had a static reference to a pipeline
object and just invoked it directly rather than invoking the one that
was installed on the SG2D it is using). All calls to pipeline objects
should be using the paradigm of "sg2d.fooPipe.Render(sg2d, ...)".
- A race condition where thread A is validating the pipeline and
installs the pipeline objects but hasn't yet reached the code to install
the loops while thread B starts rendering using that SG2D thus invoking
an operation on a partially initialized pipeline - in this case the NPE
is appropriate and allowed since we don't support multi-threaded use of
the Graphics2D objects.
...jim
Mario Torre wrote:
Il giorno ven, 12/06/2009 alle 18.41 -0700, Phil Race ha scritto:
Mario,
Did you, or can you, share a test case (and any platform specific info
needed) to repro this?
-phil.
Hi Phil!
I think I didn't explained myself very well :)
The purpose of my change is to remove the line of code that initialises
the loops from the constructor:
@@ -254,11 +254,10 @@
font = f;
if (font == null) {
font = defaultFont;
}
-loops = sd.getRenderLoops(this);
setDevClip(sd.getBounds());
invalidatePipe();
}
protected Object clone() {
The reason I want to do this is to avoid a reference to "this"
to be passed to an external class because SG2D may not be fully initialised,
and I would say that this is at least non nice :)
As for the NPE, I was referring to the fact that just removing the call to
sd.getRenderLoops(this) (which is, again, what I want to do), leaves the loops
uninitialised. This happens in the Java2D demo, for example, because validate
is not called in all the cases as the first thing.
A solution could be to put a check for null in checkFontInfo, like Jim
suggested,
because this is called before validate.
I'll prepare a test case for this issue based on the J2D demo,
but of course it's only useful to show the NPE if you remove the
sd.getRenderLoops(this) line from the constructor.
I hope this clarifies a bit my idea.
Cheers,
Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hi Mario, How are the drawGlyphList methods called when the loops is null? I ask because they are only ever installed on the SunGraphics2D object by virtue of a call to validatePipe() and all calls to validatePipe() should set the loops. So, there is a bug somewhere that causes these loops to be installed without a full validate process. As I said in the email you quoted. All calls from pipelines (GlyphListLoopPipe and AATextRenderer are both pipeline objects) should be safe because all calls to validatePipe() should set the loops. Having said that I note that there are some pipelines that do not require loops and it would be OK for a call to validate that only uses such "non-loop-based" pipelines to leave the loops field uninitialized, but all validate calls which use loop-based pipes must update the loops field. Eliminating all of those uses of loops we then fall into the case where the usage in checkFontInfo is the only usage that does not occur from a pipeline... ...jim Mario Torre wrote: Il giorno mer, 10/06/2009 alle 03.02 -0700, Jim Graham ha scritto: What is the need for this fix? Is there a bug being fixed here other than you don't like the look of the code? The reason I ask is that your fix requires a method call with a test for every graphics operation. I'd prefer if we didn't add overhead unless it was necessary for correct function. Also, the partially initialized state issue - while that technique can "in general" lead to issues, I don't think it is problematic here. If that is the concern then we could target removing just that line. Any references to the field from pipeline objects is safe since they won't be installed until a validate() is called which always sets the loops. The only suspect reference to loops would be the call from checkFontInfo() which might be invoked before the first validate() so it would need the protection against null, but almost no other piece of code you've modified can ever encounter a null loops field (or if it does then some validate() code forgot to set it)... ...jim Hi Jim! Thanks for the kind reply. I was tracking a bug in our SDL backend when I put my eyes on this code, but the bug itself is not related, just thought that this kind of code leads to unexpected errors (I shoot myself in the foot already with this stuff sometime ago...). I noticed that the references to this variable are not always protected by a call to validate though. If I don't set the loop in the constructor, and don't check for null in the getter, I get NPE in various places, for example: sun.java2d.pipe.GlyphListLoopPipe.drawGlyphList sun.java2d.pipe.AATextRenderer.drawGlyphList I get those two with the Java2D demo, but there are other places that blindly just use the loop (in fact, everywhere loops is referenced, it is just used without checking for null), and they trust on the fact that loops is just never null. There are two solution to this in my mind, either check for null everywhere loops is used (which is what I proposed with the getter) or selectively check for null in places we know it may be null (for example either in AATextRenderer.drawGlyphList, GlyphListLoopPipe.drawGlyphList, or in general in the various calls inside SunGraphics2D that end up in those methods). The third solution, that works so far, is to protect checkFontInfo() with a null check like you proposed, because in fact checkFontInfo is called before validate, and initialise there a valid instance of the RenderLoops, if they are null, which is the best option for performances, too. To be honest, those solutions looks a bit hacky to me, because we end up checking in places where "it may be used" other than in places where "it is actually used", but for the sake of saving few bytecodes and a method invocation, maybe we can go this path. Or, because it seems I opened a can of worm, I understand if you don't want to fix this issue at all ;) I have prepared a new webrew with the proposed fix, where I check for null in checkFontInfo: http://cr.openjdk.java.net/~neugens/100068/webrev.02/ I added a small comment to make clear that this guy may be null if not set via validate, checkFontInfo or setLoops. Hope this helps! Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno ven, 12/06/2009 alle 18.41 -0700, Phil Race ha scritto:
> Mario,
>
> Did you, or can you, share a test case (and any platform specific info
> needed) to repro this?
>
> -phil.
Hi Phil!
I think I didn't explained myself very well :)
The purpose of my change is to remove the line of code that initialises
the loops from the constructor:
@@ -254,11 +254,10 @@
font = f;
if (font == null) {
font = defaultFont;
}
-loops = sd.getRenderLoops(this);
setDevClip(sd.getBounds());
invalidatePipe();
}
protected Object clone() {
The reason I want to do this is to avoid a reference to "this"
to be passed to an external class because SG2D may not be fully initialised,
and I would say that this is at least non nice :)
As for the NPE, I was referring to the fact that just removing the call to
sd.getRenderLoops(this) (which is, again, what I want to do), leaves the loops
uninitialised. This happens in the Java2D demo, for example, because validate
is not called in all the cases as the first thing.
A solution could be to put a check for null in checkFontInfo, like Jim
suggested,
because this is called before validate.
I'll prepare a test case for this issue based on the J2D demo,
but of course it's only useful to show the NPE if you remove the
sd.getRenderLoops(this) line from the constructor.
I hope this clarifies a bit my idea.
Cheers,
Mario
--
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF
USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt
Please, support open standards:
http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Mario, Did you, or can you, share a test case (and any platform specific info needed) to repro this? -phil. Mario Torre wrote: Il giorno ven, 12/06/2009 alle 10.13 -0700, Phil Race ha scritto: If I don't set the loop in the > constructor, and don't check for null in the getter, I get NPE in > various places, Isn't that just a bug in (I guess) your SurfaceData subclass ? -phil. Hi Phil! No, because this is with the "plain" OpenJDK and not with my SurfaceData. This is because all the users of this field expect a valid non null value, but if we don't set it in the constructor, only the calls that happens after validate() will see a non null value. Having this null check in checkFontInfo also makes the NPE disappear in the Java2D demo, thus obtaining the same effect as initialising in the constructor, but without exposing "this". It has to be checked if there are other possible location where this variable is used non initialised, but I doubt. It's not really robust though if we depend on this initialisation in the constructor the way it is in my opinion. Not that is a serious bug, and in fact we could even say it's not a bug because the code works, but it's not correct in my point of view and looks easy to fix, so... Just want to be sure that the proposed solution doesn't add new bugs in some corner cases or doesn't add overhead, so who knows the code better has the last word on that, of course. Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno ven, 12/06/2009 alle 10.13 -0700, Phil Race ha scritto: > > If I don't set the loop in the > > constructor, and don't check for null in the getter, I get NPE in > > various places, > > Isn't that just a bug in (I guess) your SurfaceData subclass ? > > -phil. Hi Phil! No, because this is with the "plain" OpenJDK and not with my SurfaceData. This is because all the users of this field expect a valid non null value, but if we don't set it in the constructor, only the calls that happens after validate() will see a non null value. Having this null check in checkFontInfo also makes the NPE disappear in the Java2D demo, thus obtaining the same effect as initialising in the constructor, but without exposing "this". It has to be checked if there are other possible location where this variable is used non initialised, but I doubt. It's not really robust though if we depend on this initialisation in the constructor the way it is in my opinion. Not that is a serious bug, and in fact we could even say it's not a bug because the code works, but it's not correct in my point of view and looks easy to fix, so... Just want to be sure that the proposed solution doesn't add new bugs in some corner cases or doesn't add overhead, so who knows the code better has the last word on that, of course. Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
> If I don't set the loop in the > constructor, and don't check for null in the getter, I get NPE in > various places, Isn't that just a bug in (I guess) your SurfaceData subclass ? -phil. Mario Torre wrote: Il giorno mer, 10/06/2009 alle 03.02 -0700, Jim Graham ha scritto: What is the need for this fix? Is there a bug being fixed here other than you don't like the look of the code? The reason I ask is that your fix requires a method call with a test for every graphics operation. I'd prefer if we didn't add overhead unless it was necessary for correct function. Also, the partially initialized state issue - while that technique can "in general" lead to issues, I don't think it is problematic here. If that is the concern then we could target removing just that line. Any references to the field from pipeline objects is safe since they won't be installed until a validate() is called which always sets the loops. The only suspect reference to loops would be the call from checkFontInfo() which might be invoked before the first validate() so it would need the protection against null, but almost no other piece of code you've modified can ever encounter a null loops field (or if it does then some validate() code forgot to set it)... ...jim Hi Jim! Thanks for the kind reply. I was tracking a bug in our SDL backend when I put my eyes on this code, but the bug itself is not related, just thought that this kind of code leads to unexpected errors (I shoot myself in the foot already with this stuff sometime ago...). I noticed that the references to this variable are not always protected by a call to validate though. If I don't set the loop in the constructor, and don't check for null in the getter, I get NPE in various places, for example: sun.java2d.pipe.GlyphListLoopPipe.drawGlyphList sun.java2d.pipe.AATextRenderer.drawGlyphList I get those two with the Java2D demo, but there are other places that blindly just use the loop (in fact, everywhere loops is referenced, it is just used without checking for null), and they trust on the fact that loops is just never null. There are two solution to this in my mind, either check for null everywhere loops is used (which is what I proposed with the getter) or selectively check for null in places we know it may be null (for example either in AATextRenderer.drawGlyphList, GlyphListLoopPipe.drawGlyphList, or in general in the various calls inside SunGraphics2D that end up in those methods). The third solution, that works so far, is to protect checkFontInfo() with a null check like you proposed, because in fact checkFontInfo is called before validate, and initialise there a valid instance of the RenderLoops, if they are null, which is the best option for performances, too. To be honest, those solutions looks a bit hacky to me, because we end up checking in places where "it may be used" other than in places where "it is actually used", but for the sake of saving few bytecodes and a method invocation, maybe we can go this path. Or, because it seems I opened a can of worm, I understand if you don't want to fix this issue at all ;) I have prepared a new webrew with the proposed fix, where I check for null in checkFontInfo: http://cr.openjdk.java.net/~neugens/100068/webrev.02/ I added a small comment to make clear that this guy may be null if not set via validate, checkFontInfo or setLoops. Hope this helps! Cheers, Mario
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Il giorno mer, 10/06/2009 alle 03.02 -0700, Jim Graham ha scritto: > What is the need for this fix? Is there a bug being fixed here other > than you don't like the look of the code? > > The reason I ask is that your fix requires a method call with a test for > every graphics operation. I'd prefer if we didn't add overhead unless > it was necessary for correct function. > > Also, the partially initialized state issue - while that technique can > "in general" lead to issues, I don't think it is problematic here. If > that is the concern then we could target removing just that line. Any > references to the field from pipeline objects is safe since they won't > be installed until a validate() is called which always sets the loops. > The only suspect reference to loops would be the call from > checkFontInfo() which might be invoked before the first validate() so it > would need the protection against null, but almost no other piece of > code you've modified can ever encounter a null loops field (or if it > does then some validate() code forgot to set it)... > > ...jim Hi Jim! Thanks for the kind reply. I was tracking a bug in our SDL backend when I put my eyes on this code, but the bug itself is not related, just thought that this kind of code leads to unexpected errors (I shoot myself in the foot already with this stuff sometime ago...). I noticed that the references to this variable are not always protected by a call to validate though. If I don't set the loop in the constructor, and don't check for null in the getter, I get NPE in various places, for example: sun.java2d.pipe.GlyphListLoopPipe.drawGlyphList sun.java2d.pipe.AATextRenderer.drawGlyphList I get those two with the Java2D demo, but there are other places that blindly just use the loop (in fact, everywhere loops is referenced, it is just used without checking for null), and they trust on the fact that loops is just never null. There are two solution to this in my mind, either check for null everywhere loops is used (which is what I proposed with the getter) or selectively check for null in places we know it may be null (for example either in AATextRenderer.drawGlyphList, GlyphListLoopPipe.drawGlyphList, or in general in the various calls inside SunGraphics2D that end up in those methods). The third solution, that works so far, is to protect checkFontInfo() with a null check like you proposed, because in fact checkFontInfo is called before validate, and initialise there a valid instance of the RenderLoops, if they are null, which is the best option for performances, too. To be honest, those solutions looks a bit hacky to me, because we end up checking in places where "it may be used" other than in places where "it is actually used", but for the sake of saving few bytecodes and a method invocation, maybe we can go this path. Or, because it seems I opened a can of worm, I understand if you don't want to fix this issue at all ;) I have prepared a new webrew with the proposed fix, where I check for null in checkFontInfo: http://cr.openjdk.java.net/~neugens/100068/webrev.02/ I added a small comment to make clear that this guy may be null if not set via validate, checkFontInfo or setLoops. Hope this helps! Cheers, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
Re: [OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
What is the need for this fix? Is there a bug being fixed here other than you don't like the look of the code? The reason I ask is that your fix requires a method call with a test for every graphics operation. I'd prefer if we didn't add overhead unless it was necessary for correct function. Also, the partially initialized state issue - while that technique can "in general" lead to issues, I don't think it is problematic here. If that is the concern then we could target removing just that line. Any references to the field from pipeline objects is safe since they won't be installed until a validate() is called which always sets the loops. The only suspect reference to loops would be the call from checkFontInfo() which might be invoked before the first validate() so it would need the protection against null, but almost no other piece of code you've modified can ever encounter a null loops field (or if it does then some validate() code forgot to set it)... ...jim Mario Torre wrote: Hello all! While hacking on Cacio I've found that SunGraphics2D exposes a reference to "this" inside the constructor to another class, while initialising a field that contains the RederingLoops. I filed a bug report and proposed a patch for review: https://bugs.openjdk.java.net/show_bug.cgi?id=100068 The webrew is here: http://cr.openjdk.java.net/~neugens/100068/webrev.01/ There is not much to say about the rationale for the bug/fix, just that the code looks a bit borked to me with those public references (there are others around, I think I should fix them all at some point), but the real problem is indeed exposing "this" in the constructor. I hope I did the webrew correctly, as I had other patches around and no patch queue on this tree (yeah, yeah, I know...) so I had to do some manual tricks to make webrew happy, but the patch should be complete. I tested with all the swing apps that come with OpenJDK, as well as those two nice things: http://java.sun.com/products/java-media/2D/samples/index.html I'll bug you every day if you make me wait too much about that review :) Have fun, Mario
[OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised
Hello all! While hacking on Cacio I've found that SunGraphics2D exposes a reference to "this" inside the constructor to another class, while initialising a field that contains the RederingLoops. I filed a bug report and proposed a patch for review: https://bugs.openjdk.java.net/show_bug.cgi?id=100068 The webrew is here: http://cr.openjdk.java.net/~neugens/100068/webrev.01/ There is not much to say about the rationale for the bug/fix, just that the code looks a bit borked to me with those public references (there are others around, I think I should fix them all at some point), but the real problem is indeed exposing "this" in the constructor. I hope I did the webrew correctly, as I had other patches around and no patch queue on this tree (yeah, yeah, I know...) so I had to do some manual tricks to make webrew happy, but the patch should be complete. I tested with all the swing apps that come with OpenJDK, as well as those two nice things: http://java.sun.com/products/java-media/2D/samples/index.html I'll bug you every day if you make me wait too much about that review :) Have fun, Mario -- Mario Torre, Software Developer, http://www.jroller.com/neugens/ aicas Allerton Interworks Computer Automated Systems GmbH Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany http://www.aicas.com * Tel: +49-721-663 968-44 pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim Geschäftsführer: Dr. James J. Hunt Please, support open standards: http://endsoftpatents.org/
