New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8238954: Improve performance of tiled snapshot rendering #112
Conversation
…ger than max texture size" This reverts commit 1823f6e
…arger than max texture size
👋 Welcome back fthevenet! A progress list of the required criteria for merging this PR into |
I've marked this PR as a WIP for the time being because I've only tested it on the d3d rendering pipeline so far, so I think it is too early to call for a formal review yet. In a nutshell, this is what this PR does:
TODO:
|
/reviewers 2 |
@kevinrushforth |
Webrevs
|
I finally got a chance to do some more extensive testing when running this patch with the es2 pipeline on Linux. |
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
…set to false if needed.
On My windows10 machine, I can observe 20 to 30 % reduction in time to take snapshot. |
14-ea+9
|
14-internal
|
15-internal:
|
I've uploaded 3 sets of results, from 3 different implementations:
My conclusion from the above results is twofold:
|
By the way, the code for the above benchmark is available here: https://github.com/fthevenet/snapshot-perf-meter Would it make sense to include it into the OpenJFX repo, maybe under jfx/apps/performance ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change seems good to me. Suggested few minor corrections and a query.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Show resolved
Hide resolved
pImage.setImage(com.sun.prism.Image.fromIntArgbPreData(IntBuffer.allocate(w * h), w, h)); | ||
} | ||
// Find out it is is possible to divide up the image in tiles of the same size | ||
AtomicBoolean exactWidthDivFound = new AtomicBoolean(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactWidthDivFound
is unused, may be can skip creating a named variable and just pass a unnamed AtomicBoolean
in next call to computeOptimumTileSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a change so that null can be passed to the method if the AtomicBoolean instance is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good to me, Please revert the changes in import.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
Now that the tiling is done in the |
It won't help with things in their current state, as the What do you all think ? |
At first glance, the NPE in JDK-8189082 occurs in the Prism layer, which is one level below Quantum where the tiling is currently implemented, so I'm not sure tit is reachable from there; if we want the code to be shared, it looks like it would need to be moved even further down (maybe in the Also, while reusing code is generally the way to go, in such lower layers, very closely intertwined with the actual rendering, I'm afraid that insisting on having a "one-size-fits-all" implementation might get in the way of necessary case-by-case optimizations, so I'd like to have someone with a deeper knowledge of the code base to weight in before starting work in that direction. Maybe @kevinrushforth could advise? |
Hi everyone, This PR hasn't seen much activity in a while, so I though I would give it a gentle kick to hopefully get it moving again ;) Thanks. |
Sorry for the delay. This is on my review queue, which has been growing of late. I'll take a look at it soon. To answer one of your questions:
I think your idea of addressing the other similar cases (Canvas and SubScene in particular) in a follow-up PR seems fine. If, at that time, you find you can refactor this to share some of the implementation, that would be good. |
Actually, it also fails the same way on Ubuntu 20.04 as it does on macOS if I force HW acceleration on Ubuntu. It's off by default when running in a VM, so is using the SW pipeline, which doesn't have a hard limit (so that could explain the heap problem you were facing). Ensuring that the tile size is > 0 fixes it on Linux and Mac. |
I've pushed the changes discussed above, and verified that the tests now pass on a Ubuntu 20.04 VM with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've finished my testing on 4 platforms: Windows 10, macOS 10.15, Ubuntu 16.10 (physical), Ubuntu 20.04 (VM with forceGPU set to true). All looks good.
The code looks good too with a couple questions below.
tests/system/src/test/java/test/javafx/scene/Snapshot2Test.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me too, suggested minor change.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
- Fixed comments in QuantumToolkit - Only initialize RTT cache if needed
@fthevenet This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type
Since the source branch of this PR was last updated there have been 131 commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@arapte, @kevinrushforth) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
I have read Ambarish's code more closely and I am now convinced it is indeed easier to understand; beyond the change from "while" to "for" loops, it introduces an extra set of variables which avoids the same "m/b/rTileWidth/Height" vars to have to designate different things depending in which loop they're used (M, B, or R). In hindsight, this is why I had such a hard time naming them. I have run the automated tests on Windows and Ubuntu (w/ forceGPU=true) and it passes. The benchmark I made also produces similar results. If it's not too much of a problem now that the PR's been approved, I think it would beneficial to make the change, especially considering that this bit of code might be used as a starting point for other issues requiring tiling. |
That would be fine with me. |
I'll do a quick sanity test on all three platforms. |
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Sanity tested on all three platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified On Mac and Windows10, looks good to me.
/integrate |
@fthevenet |
/sponsor |
@kevinrushforth @fthevenet The following commits have been pushed to master since your change was applied:
Your commit was automatically rebased without conflicts. Pushed as commit 32584db. |
Issue JDK-8088198, where an exception would be thrown when trying to capture a snapshot whose final dimensions would be larger than the running platform's maximum supported texture size, was addressed in openjfx14.
The fix, based around the idea of capturing as many tiles of the maximum possible size and re-compositing the final snapshot out of these, is currently only attempted after the original, non-tiled, strategy has already failed.
This was decided to avoid any risk of regressions, either in terms of performances and correctness, while still offering some relief to the original issue.
This follow-on issue aims to propose a fix to the original issue, that is able to correctly decide on the best snapshot strategy (tiled or not) to adopt before applying it and ensure best performances possible when tiling is necessary while still introducing no regressions compared to the original solution.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jfx pull/112/head:pull/112
$ git checkout pull/112