RFR: 8238954: Improve performance of tiled snapshot rendering

thevenet.fred at free.fr thevenet.fred at free.fr
Mon Jun 29 12:40:35 UTC 2020 

Hi Ambarish,
Thanks for your review.

I don't have access to a Mac so I can't check that directly.
The tests pass on both my Windows 10 and Ubuntu 20.04 environments
and the stack isn't very helpful (it simply indicates the 
renderImage method returned null).
There could be many reasons for that, but this is what would 
happen when running the test with the unpatched version of the 
QuantumToolkit::renderToImage method. 
Maybe double-check the pull applied cleanly on your clone?

With regard to the choice of random noise for the test image,
my idea was to be able to catch any misalignment caused by tiling
in the final snapshot: using a single color or a simple pattern
would not necessarily help catching such errors.
Using a complex image could do the trick, and avoid the broken aspect 
you mentioned, but it would need to be very large (>4096x4096), 
and I was not sure it would be wise to add such a large binary resource
to the repo.
So I settled for random noise, since it was the simplest way to generate 
an image with 100% guaranties any misalignment would be caught by
comparing pixels 1 to 1.

PS: I cc'ing the mailing list, so that the skara bot will hopefully pick 
it up and add it to the PR, so we can resume the discussion there once 
Github is up again.

-- Fred.







----- Mail original -----
De: "Ambarish Rapte" <ambarish.rapte at oracle.com>
À: "Kevin Rushforth" <kevin.rushforth at oracle.com>, "thevenet fred" <thevenet.fred at free.fr>
Envoyé: Lundi 29 Juin 2020 13:12:47
Objet: RE: RFR: 8238954: Improve performance of tiled snapshot rendering

Hi, 
Looks like github is down, Unable to do a fresh login OR post a comment on the PR from already logged in session.
Here is my comment about the tests,

I observed that the added tests are failing on mac machine(Mojave 10.14.6), but they do pass on windows10. Can you please verify ? Attached mac test execution log.

I would suggest to fill the image with a single or preferably some pattern of multiple recognizable colors instead of noise. The noise gives a broken feel. It might confuse the verifications of any future fixes in this area. A well defined color would be nice.


Regards,
Ambarish

-----Original Message-----
From: Kevin Rushforth 
Sent: Sunday, June 28, 2020 1:27 AM
To: thevenet.fred at free.fr
Cc: Ambarish Rapte <ambarish.rapte at oracle.com>
Subject: Re: RFR: 8238954: Improve performance of tiled snapshot rendering

No worries.

I hope to finish the review on Monday or Tuesday. I ran a full test on Windows today and it looks good. I have some more testing to do and then I will sanity test it on other platforms. I took a quick look at the code changes since last time, and didn't spot any issues. I'll do a more careful review Monday. The new tests look good, but I haven't looked carefully at the code yet.

-- Kevin


On 6/26/2020 8:28 AM, thevenet.fred at free.fr wrote:
>> I didn't copy openjfx-dev, so there is no way my reply could have 
>> been forwarded to either the list or the PR (else it would have been).
> Yes, of course: I realized that immediately *after* I pressed the send button...
>
>> I don't see any duplication of your message. There is exactly one 
>> reply to the list, which was forwarded to the PR (as expected). The 
>> Skara bot will forward comments made in the GitHub PR as a reply to 
>> the RFR email and vice-versa.
>>
>> If you want to make a comment on the mailing list related to your PR, 
>> but don't want that to show up in the PR, the easiest way is to start 
>> a new message thread rather than doing a reply.
> Indeed, there's no need to overthink it.
>
> Thank you, and my apologies for being a little dense today ;-)
>
> -- Fred
>
>
>> -- Kevin
>>
>>
>> On 6/26/2020 7:57 AM, thevenet.fred at free.fr wrote:
>>> Hi Kevin, thank you for that.
>>>
>>> As an unrelated side question: my original email to the list got 
>>> picked up by a bot and pushed to the PR, and then pushed back onto 
>>> the list,adding even more noise.
>>> Meanwhile your reply didn't; why is that?
>>>
>>> Do you know where I can find the rules used by the gh bot to pick-up 
>>> messages from the list, so I can avoid tripping one of them 
>>> unintentionally?
>>>
>>> Thanks.
>>>
>>> -- Fred
>>>
>>>
>>>
>>> ----- Mail original -----
>>> De: "Kevin Rushforth" <kevin.rushforth at oracle.com>
>>> À: "thevenet fred" <thevenet.fred at free.fr>
>>> Cc: "Ambarish Rapte" <ambarish.rapte at oracle.com>
>>> Envoyé: Vendredi 26 Juin 2020 15:39:56
>>> Objet: Re: RFR: 8238954: Improve performance of tiled snapshot 
>>> rendering
>>>
>>> Thanks for the reminder. Ambarish and I spoke yesterday and both of 
>>> us will review it.
>>>
>>> -- Kevin
>>>
>>>
>>> On 6/26/2020 2:13 AM, thevenet.fred at free.fr wrote:
>>>> Hi everyone,
>>>>
>>>> I think this is now ready for another (and hopefully final) round of reviews.
>>>>
>>>> Thanks.
>>>>
>>>> -- Fred
>>>>
>>>> ----- Mail original -----
>>>> De: "Frederic Thevenet" 
>>>> <github.com+7450507+fthevenet at openjdk.java.net>
>>>> À: "openjfx-dev" <openjfx-dev at openjdk.java.net>
>>>> Envoyé: Vendredi 28 Février 2020 18:58:00
>>>> Objet: RFR: 8238954: Improve performance of tiled snapshot 
>>>> rendering
>>>>
>>>> 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.
>>>>
>>>> -------------
>>>>
>>>> Commits:
>>>>     - e13b5a06: Use tiling in QuantumToolkit::renderToImage when 
>>>> requested image is larger than max texture size
>>>>     - b914a48d: Revert "8088198: Exception thrown from snapshot if 
>>>> dimensions are larger than max texture size"
>>>>
>>>> Changes: https://git.openjdk.java.net/jfx/pull/112/files
>>>>     Webrev: https://webrevs.openjdk.java.net/jfx/112/webrev.00
>>>>      Issue: https://bugs.openjdk.java.net/browse/JDK-8238954
>>>>      Stats: 173 lines in 2 files changed: 84 ins; 50 del; 39 mod
>>>>      Patch: https://git.openjdk.java.net/jfx/pull/112.diff
>>>>      Fetch: git fetch https://git.openjdk.java.net/jfx 
>>>> pull/112/head:pull/112
>>>>
>>>> PR: https://git.openjdk.java.net/jfx/pull/112

More information about the openjfx-dev mailing list