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

[Frederic Thevenet]

On Sat, 16 May 2020 14:20:10 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>>> 
>>> 
>>> 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.

I have re-written the way we walk through tiles as per the discussion above.
It isn't really more concise than the previous iteration but it is probably more straight forward and less surprising.
It also handles more cases with the least amount of tile resizing than the previous code, so I think it is a clear
improvement. What I'm not terribly happy with is the variable names: I must admit I quickly ran out of idea for naming
the various instances of tiles sizes and offsets, so I reused the "M, R, B, C" terminology used above. However I'm
worried it might seem a bit obscure when taken out of the context of this discussion, so if anyone have better names to
suggest, I'd be happy to change these.

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

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