Re: [PATCH] 8088147: FXCanvas: implement custom cursors (revised)
Hi Kevin, all,
attached please find an updated patch and my replies to Kevin’s comments at
https://bugs.openjdk.java.net/browse/JDK-8088147:
> build.gradle
>
> 1. In the following:
>
> + if (IS_JIGSAW_TEST) {
> + enabled = false // FIXME: JIGSAW -- support this with modules
> + logger.info("JIGSAW Testing disabled for swt")
>
> I verified that it correctly skips this in JIGSAW mode. Can you look into
> implementing this, though? It should not be difficult, and once we switch to
> only supporting Jigsaw mode the tests will never be run until this is done.
> If it does turn out to be too much work, then we could consider it as a
> follow-on.
>
As already indicated in an earlier mail I would prefer to treat building up
JIGSAW tests as a follow-on. I have not gained much experience with JIGSAW yet,
but I would be willing to support this as far as I can. The problem I currently
see is that SWT is still an automatic module, and JIGSAW-based SWT tests would
have to access code from the swt-debug.jar, which is as well an automatic
module. AFAIK, we would have to turn SWT into a named module first in order to
make swt-debug.jar available on its module path. That seems to be out of scope
here and should probably be investigated in an own issue.
> 2. Platform logic:
>
> + if(IS_MAC){
> + enabled = false
> ...
> + }
> + if(IS_LINUX){
>
> Normally we would handle this in the test itself by using assumeTrue and
> platform checks. In this case, though, since it applies to all SWT tests run
> on Mac (or Linux in case of the warning), this seems fine.
>
> Please fix the spacing to match our coding conventions, though. There should
> be a space after the 'if' and a space before the '{'. So:
>
> if (IS_MAC) {
Ok. fine. We could even try to get SWT tests working on Mac by falling back to
ant-based test execution here, but I would propose to handle this in a
different issue as well (after all, this issue is about SWT image cursors and
not about building up an SWT test harness).
> modules/swt/src/main/java/javafx/embed/swt/SWTCursors.java
>
> 3. Spacing issues:
>
> + if(display == null){
>
> Please add a space after '(' and before '}'
>
>
> -}
> +}
> \ No newline at end of file
>
> Please restore the newline after the last line.
Done.
> SWTCursorsTest.java
>
> 4. Need a blank line before the package declaration:
>
> + */
> +package test.javafx.embed.swt;
> +
Done.
> 5. Can you sort the imports alphabetically?
I organized the imports and removed unused ones. It seems they are pretty much
consistent with those of the other SWT classes.
> 6. Since the test loops waiting on a latch, please add a 10 second timeout
> for the test:
>
> @Test(timeout=1)
> public void testImageCursor() throws Throwable {
Done.
> SWTImageCursorTest.java
>
> 7. The following can use a lambda (as the setOnMouseEntered already did).
>
> + rect.setOnMouseExited(new EventHandler() {
> + @Override
> + public void handle(MouseEvent event) {
> + scene.setCursor(null);
> + }
> + });
Done.
Kevin, thanks for picking this up.
Regards,
Alexander
> Am 27.07.2016 um 01:15 schrieb Kevin Rushforth :
>
> Picking this back up again, I have just added my comments to JBS issue.
>
> https://bugs.openjdk.java.net/browse/JDK-8088147
>
> The comments are minor in scope, so I suspect it will be ready for final
> review after one more iteration. It will need one other reviewer, since I am
> sponsoring the change.
>
> Could someone else review this as well (maybe Alexander Z or Sergey)?
>
> -- Kevin
>
>
> Kevin Rushforth wrote:
>> I'm back, and given that the review will take some time anyway, I will
>> sponsor this once the review is complete. I see that Guru uploaded the patch
>> for you (thanks, Guru) so I can test it next week.
>>
>> -- Kevin
>>
>>
>> Alexander Nyssen wrote:
>>> It seems I was unsuccessful again. If somebody would be willing to sponsor
>>> this contribution while Kevin is away (or at least update the patch
>>> provided for JDK-8088147), I could mail the patch privately.
>>>
>>> Regards,
>>> Alexander
>>>
>>>
Am 11.07.2016 um 19:59 schrieb Alexander Nyssen :
It seems my attachment has again been ‚consumed‘ by the list. Trying again
with an archive containing the patch file.
Regards,
Alexander
> Am 08.07.2016 um 23:28 schrieb Alexander Nyssen :
>
> Hi Kevin, all,
> attached please find a revised patch, where I have added the required
> export to PlatformImpl and have fixed the build script, so it does no
> longer break when being executed in jigsaw mode.
> As swt is no named module yet, I have disabled the JUnit test in jigsaw
> mode for now. We would probably have to set up swt as a named module to
> enable this, and would further have to make swt-debug.jar available as an
> automated module on its module path. But this is a diff
Re: [PATCH] 8088147: FXCanvas: implement custom cursors (revised)
Picking this back up again, I have just added my comments to JBS issue. https://bugs.openjdk.java.net/browse/JDK-8088147 The comments are minor in scope, so I suspect it will be ready for final review after one more iteration. It will need one other reviewer, since I am sponsoring the change. Could someone else review this as well (maybe Alexander Z or Sergey)? -- Kevin Kevin Rushforth wrote: I'm back, and given that the review will take some time anyway, I will sponsor this once the review is complete. I see that Guru uploaded the patch for you (thanks, Guru) so I can test it next week. -- Kevin Alexander Nyssen wrote: It seems I was unsuccessful again. If somebody would be willing to sponsor this contribution while Kevin is away (or at least update the patch provided for JDK-8088147), I could mail the patch privately. Regards, Alexander Am 11.07.2016 um 19:59 schrieb Alexander Nyssen : It seems my attachment has again been ‚consumed‘ by the list. Trying again with an archive containing the patch file. Regards, Alexander Am 08.07.2016 um 23:28 schrieb Alexander Nyssen : Hi Kevin, all, attached please find a revised patch, where I have added the required export to PlatformImpl and have fixed the build script, so it does no longer break when being executed in jigsaw mode. As swt is no named module yet, I have disabled the JUnit test in jigsaw mode for now. We would probably have to set up swt as a named module to enable this, and would further have to make swt-debug.jar available as an automated module on its module path. But this is a different problem. Regards, Alexander Am 30.06.2016 um 12:14 schrieb Alexander Nyssen : Hi Kevin, Am 30.06.2016 um 01:20 schrieb Kevin Rushforth : Hi Alexander, I attached the patch to the bug: https://bugs.openjdk.java.net/browse/JDK-8088147 If I build it and run the manual test in "legacy" mode, meaning run everything with 9+109 and the legacy jfxrt.jar file, then it runs and the cursor now changes. So this looks like a good starting point for a fix. Fine. I tried building and running this in Jigsaw mode (building with jdk-9+109, but running the tests with a more recent JDK that includes modularization support), and noticed two problems right away that must be addressed: 1. The unit tests for SWT are missing some of the jigsaw test tasks so the build fails right away with an exception from gradle: Task with name 'jigsawTestsLinux' not found in project ':swt'. Wiring up SWT-based tests to our unit test harness will take a bit more work than what you have provided (not even counting the Mac issue, which could be handled by excluding the test on Mac). In the short term, relying on manual tests for this fix might be best. I did not execute the tests in jigsaw mode yet, because other tests failed in this mode, too (as indicated in an earlier discussion). I will try to set things up in a virtual machine with Windows and/or Linux so I can work on the Gradle tests without having to deal with the Mac issue. The test harness will IMHO also be required for other contributions, and it would of course be fine if the automated test, I included in this patch, could be executed as well. 2. You have introduced a dependency on a new internal package, com.sun.javafx.tk. If this is required in order to implement the fix, then you will need to add this package to the list of packages exports to javafx.swt in PlatformImpl; otherwise the following exception is thrown at runtime: Exception in thread "JavaFX Application Thread" java.lang.IllegalAccessError: class javafx.embed.swt.SWTCursors (in module javafx.swt) cannot access class com.sun.javafx.tk.Toolkit (in module javafx.graphics) because module javafx.graphics does not export com.sun.javafx.tk to module javafx.swt This dependency is required unless there is public API to convert a platform image (which is provided by the image cursor frame) to an image. To me, Image image = Toolkit.getImageAccessor().fromPlatformImage(cursorFrame.getPlatformImage()); seemed to be the way to go. I will thus add the respective export in a revised patch. I won't have time to sponsor this change until the second half of July, but if others have time, the review can proceed and I'll pick it back up then if it is in good enough shape to run. Especially setting up the SWT test harness will be kind of a blocker for succeeding contributions, so it would be nice if somebody could step in. Regards, Alexander -- Kevin Kevin Rushforth wrote: Hi Alexander, It looks like your patch was stripped out by the mailing list server. Can you please send me the patch offline, as a zip file (so line endings are preserved across different systems), and I will unzip it and attach it to the bug report. -- Kevin Alexander Nyssen wrote:
Re: JDK-8153350 - JavaFX webstart with javaws.exe selects wrong toolkit (unnecessary relaunch)
> Hi, > > it may be unrelated to these relaunches, but I have been wondering for > awhile, what the toolkit parameter in the ant element is > supposed to do. > > see: > > https://docs.oracle.com/javase/8/docs/technotes/guides/deploy/javafx_ant_task_reference.html#CIAGCAFH > > At least in my tests changing the parameter had no effect on the generated > jnlp file. Good question.. maybe Chris can answer? -DrD-
JDK-8153350 - JavaFX webstart with javaws.exe selects wrong toolkit (unnecessary relaunch)
Hi, it may be unrelated to these relaunches, but I have been wondering for awhile, what the toolkit parameter in the ant element is supposed to do. see: https://docs.oracle.com/javase/8/docs/technotes/guides/deploy/javafx_ant_task_reference.html#CIAGCAFH At least in my tests changing the parameter had no effect on the generated jnlp file. - Stefan
HEADS-UP: Massive directory renaming in openjfx/9-dev/rt planned for Monday, August 1
TO: All developers who build openjfx/9-dev As a heads-up, we are preparing to rename all directories under rt/modules this coming Monday, August 1 to address JDK-8161705 [1]. This is a step along the path to compiling with a newer, Jigsaw-compatible JDK as described in JDK-8161704 [2]. Dave will send out a review in the next day or so. The 12 directories under "rt/modules", containing a total of 22,283 files, will be renamed; this will be a *very* large changeset. Developers need to be aware of the following: 1) The repo will remain locked on Monday after our weekly sanity testing; the changeset for this fix will be the first thing pushed after integration to 9 master, and then the repo will be unlocked. 2) Any patches that you have in flight will need to be redone using the new file locations. We will provide a script to help with this. 3) Backports from 9-dev to 8u-dev will no longer import cleanly without modification due to the location changes. We will provide a script to help with this. Please let Dave or me know if there are any questions. -- Kevin [1] https://bugs.openjdk.java.net/browse/JDK-8161705 [2] https://bugs.openjdk.java.net/browse/JDK-8161704
[9] Review Request for 8162551: SwingNode scaling doesn't work
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8162551 webrev: http://cr.openjdk.java.net/~ssadetsky/8162551/webrev.00/ Upscaling is not needed to send coordinates to sun.swing.JLightweightFrame. --Semyon
Re: Scene graph performance
Hi, I agree with most that has been said here, but I want to add a few things. I've seen a number of "performance issues" with JavaFX in customer projects. Most of them are simply due to the application doing things on the FX App Thread that shouldn't be done there. Apart from these obvious things, it is indeed mainly the CSS and layout processing that consumes most of the time. I spent lots of time working on mobile performance, and in that context the CPU limitation is even more pronounced, as the GPU on modern mobile devices is pretty good. I don't have enough different devices, but to give an idea about numbers: when running a typical app, the GPU load on my desktop is about the same as the GPU load on my Nexus 5, but the CPU load on the Nexus is 5-10 times higher than on desktop. I measure this by comparing numbers in the pulseLogger. Hence, on mobile the limitation that only a single thread can do the CSS/layout phase AND the rasterisation is really a bottleneck. However, there are many tricks that can make it better. Contrary to e.g. Android and iOS, the JavaFX API's still allow for lots of flexibility that can be very helpful, but that also can easily kill performance. Caching is one of those things. If you cache the right Node, you'll gain a lot. But if you cache a Node that often becomes dirty, you screw performance. There is no general solution for this, I believe. The way to address this is to create components that work for specific usecases (e.g. cache the content of a pane while animating, and don't cache it once it might change again). This is what we do in the Gluon Mobile components. Complex CSS and layout changes are CPU hungry as well. About the latter: be careful about bounds that vary (e.g. start with a negative offset) without any visual consequence, as this will increase the dirty region. Profiling is the most important thing to do when your application is slow. Find the bottleneck, fix it, go to the next bottleneck. Each application has its own bottlenecks, but with enough time, I really believe all applications can at least be improved. One of the great things about JavaFX is the decoupling between the API and the rendering pipeline. If someone writes a renderer that works completely different from the existing ones, that is fine. All applications should be able to run with this. One of the things I'm thinking about is to see if/how Vulkan would be a good alternative to OpenGL. But that would not speed up the single-threaded bottleneck, so I applaud all initiatives in that area (e.g. do some parallel CSS processing/) - Johan On Fri, Jul 22, 2016 at 2:09 AM Scott Palmer wrote: > > > On Jul 21, 2016, at 6:18 PM, Richard Bair > wrote: > > > > Hi Steve, > > > > It could be a benchmark problem, although I wouldn’t be surprised at all > if the benchmark was exercising the platform in some way that was CPU > limited. Assuming it is CPU limited (and not going multi-core), I think the > problem really comes down to what Markus said: > > > >> The limiting factor is the single-thread architecture of rather all > parts of JavaFX. The only real difference you see between machines is not > correlating with neither number of CPU cores nor GPU cores, but only with > CPU frequency, roughly spoken. Short term fixes will only provide little > improvement, by optimizing the critical execution path (i. e. produce hot > spot histogram using a profiler), for example improvement clipping, > caching, etc. Huge performance optimizations need an architectural change > within JavaFX's "scenegraph-to-bitmapframe" (a.k.a. rendering) pipeline to > use parallel execution in lots of places. Typical design patterns would be > parallel iterations, work-stealing executors, fibers (a.k.a cooperative > multi-threading, a.k.a CompletableFuture), and last but not least > partitioned rendering (a.k.a tiled rendering). > >> > >> I am pretty sure you can add a lot more ideas to the list and produce > great performance, scaling linearly with number of CPU cores / GPU cores, > but this somes at a cost: Risk to introduce hard to track bugs, and needed > manpower. > >> > >> If somebody has at least a lot of free spare time, I am pretty sure > Kevin could easily provide a huge set of work items in this area. :-) > > > > > > JavaFX was setup to be multi-threaded — in fact there are always at > least 2 threads — the application / scene graph thread and the render > thread. Going way, way back the goal was for multi-core > computation/rasterizing on the NG side (Prism), but it didn’t get done for > a variety of reasons. I couldn’t even say what kind of performance win/loss > it would bring. I’m sure for some workloads it would be way better,but for > many others it probably wouldn’t make any difference. A lot of other more > pressing features had to be implemented first which would allow people to > build apps at all on top of FX (like controls and effects and animations > and so forth), and Prism has served us really
