[Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering

[Kevin Rushforth]

On Thu, 14 May 2020 12:48:30 GMT, Frederic Thevenet <github.com+7450507+fthevenet at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java line 1495:
>> 
>>> 1494:                 }
>>> 1495:                 //Copy tile's pixel into the target image
>>> 1496:                 targetImg.image.setPixels(xOffset, yOffset, w, h,
>> 
>> Typo: should be "pixels" (plural)
>
>> 
>> 
>> Overall, this looks quite good. In particular the tiled rendering, as implemented by the `renderTile` method, should be
>> reasonably efficient.
>> My only high-level comment is that I'm somewhat skeptical of `computeOptimumTileSize` to determine the size and
>> direction of tiling. I note that in the case of an image that is tiled in both X and Y, there are at most 4 distinct
>> tile sizes if it doesn't fit evenly. In the case where only one of X or Y is tiled, there are at most 2 distinct tile
>> sizes. Here is an example:  ``` +-----------+-----------+  .  +-------+
>> |           |           |  .  |       |
>> |     M     |     M     |  .  |   R   |
>> |           |           |  .  |       |
>> +-----------+-----------+  .  +-------+
>> |           |           |  .  |       |
>> |     M     |     M     |  .  |   R   |
>> |           |           |  .  |       |
>> +-----------+-----------+  .  +-------+
>>       .           .        .      .
>> +-----------+-----------+  .  +-------+
>> |           |           |  .  |       |
>> |     M     |     M     |  .  |   R   |
>> |           |           |  .  |       |
>> +-----------+-----------+  .  +-------+
>> |     B     |     B     |  .  |   C   |
>> +-----------+-----------+  .  +-------+
>> ```
>> 
>> Where `M` represents the middle set of tiles each with a size of `tileW x tileH`. `R` is the right hand column of
>> tiles, `B` is bottom row, and `C` is corner.
>> Recognizing this, I wonder if it might be better to always use the maximum tile size, but fill all of the middle tiles
>> of that size first, and then pick up the right and/or bottom edges as needed. This will minimize thrashing (no more
>> than 3 changes of tile size), while avoiding the more complicated logic that tries to keep the tiles all the same size
>> at the cost of smaller tiles, and which has to fall back to using uneven tiles anyway. If you do it this way, there is
>> also no need to have code that switches the order of the inner loop. It will naturally handle that.  Either way, I'd
>> like to see some additional system tests that cover all of the cases of X and Y fitting/not-fitting exactly (and if you
>> stick with your current approach, X or Y as the inner loop).  I left a couple inline comments as well.
> 
> I'll need to think about this a bit more, but maybe a good approach could be to generally adopt your solution, but
> still attempt to see if any of the snapshot's dimension  can be divided equally by 2 or 3 (while being less than
> maxTextureSize) , and use that a a tile size. As the number of tiles increases, it become less important to have same
> sized tiles as you demonstrated so using maxTextureSize should be fine.  This way we get rid of the inner loop
> direction logic (which I agree is verbose and kind of confusing), and still have a chance to optimize the case where
> tiled snapshots are made up of 4 tiles or less (which I see as being the most frequent use case, in my experience
> anyway).

That sounds like a promising approach.

-------------

PR: https://git.openjdk.java.net/jfx/pull/112