Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

On 23.01.2020 18:09, Anthony Vanelverdinghe wrote:
> On 22/01/2020 18:52, Rony G. Flatscher wrote:
... cut ...
>> Maybe one more question: there would be an optimization possible by 
>> compiling scripts for script
>> engines that have the javax.script.Compilable interface implemented and use 
>> the compiled version
>> to execute/evaluate the scripts (may be helpful for event handler code e.g. 
>> for onMouseMove event
>> handlers). Can the fix include such an optimization or should there be a 
>> separate discussion/RFE
>> for it beforehand? (Adding this would be trivial in the context of the fix, 
>> however the bug
>> description would not hint at such an optimization.)
> In my opinion, this should be filed as a separate issue, since it's unrelated 
> to the current issue
> and is an enhancement, rather than a bug.

Thank you very much Anthony, will do.

---rony

RFR: 8237770: Error creating fragment phong shader on iOS

[Jose Pereda]

This PR defines a pre-processor in the phong frag files to avoid inline 
declaration of #extension when several frags are combined that leads to the 
error:

syntax error: #extension must always be before any non-preprocessor tokens

-

Commits:
 - 14ccbe6a: Define pre-processor in frag files to avoid inline declaration of 
#extension when several frags are combined

Changes: https://git.openjdk.java.net/jfx/pull/93/files
 Webrev: https://webrevs.openjdk.java.net/jfx/93/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8237770
  Stats: 45 lines in 15 files changed: 45 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/93.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/93/head:pull/93

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

Re: [Rev 03] RFR: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

[Kevin Rushforth]

On Fri, 24 Jan 2020 14:41:00 GMT, Guru Hb  wrote:

>> Previous commits in this pull request have been removed, probably due to a 
>> force push. The incremental views will show differences compared to the 
>> previous content of the PR.
> 
> Marked as reviewed by ghb (Reviewer).

@effad you can integrate this when ready. I will sponsor it.

-

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

Indicating minimum number of reviewers for a PR [was: Skara - bot sending can-be-integrated message prematurely?]

[Kevin Rushforth]

The Skara team has implemented SKARA-217 [1] to allow for increasing the 
number of reviewers before it will mark a PR as ready for integration.


Starting from now, any "R"eviewer (that is, someone with a Reviewer role 
in the OpenJFX Project) can indicate that a PR needs 2 reviewers (or 
perhaps even 3 in some cases if one of the reviewers desires additional 
eyes on a risky PR), can issue this command in the PR:


/reviewers 2

I'll do that now for the ones that we've already identified as needing 
multiple reviewers.


Thanks to Nir Lisker for suggesting the Skara improvement, and to the 
Skara team for implementing it. Let me know if there are any questions.


-- Kevin

[1] https://bugs.openjdk.java.net/browse/SKARA-217




On 1/8/2020 10:19 AM, Nir Lisker wrote:

I forgot to mention that I filed these:

https://bugs.openjdk.java.net/browse/SKARA-218
https://bugs.openjdk.java.net/browse/SKARA-217

They weren't triaged, so maybe the Skara team needs to be notified.

On Thu, Dec 19, 2019 at 6:24 PM Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>> wrote:


FYI, for anyone interested, the Skara team submitted the following
PR to modify the "ready for integration" message:

https://github.com/openjdk/skara/pull/343

We can file a follow-up RFE to have them consider adding
"/reviewers" and "/csr" commands.

-- Kevin


On 12/18/2019 7:17 PM, Phil Race wrote:

It would have to allow anyone who has reviewer status to add that
comment.
Not just the author since if they knew we would have less need
for it.

-Phil.

On Dec 18, 2019, at 11:31 AM, Kevin Rushforth
mailto:kevin.rushfo...@oracle.com>>
wrote:


That's an interesting idea. It would, of course, need to
disallow reducing the number below the minimum specified in
.jcheck/conf (e.g., we wouldn't allow "/reviewers 0").

-- Kevin


On 12/18/2019 10:36 AM, Nir Lisker wrote:


The client libraries in the OpenJDK do as a default rule,
excusing simple fixes.


Then maybe it would be helpful to have a "/reviewers n" command
that will tell the bot how many reviewers are needed. It's less
convoluted than the CSR tracking and basically replaces the
comment a reviewer would make anyway to inform the PR author
how many reviewers they should wait for. Extending the bot to
look for n reviewers instead of the current 1 should not be far
fetched.



On Wed, Dec 18, 2019 at 4:02 AM Philip Race
mailto:philip.r...@oracle.com>> wrote:



On 12/16/19, 4:14 PM, Nir Lisker wrote:
> Do other projects also have multi-reviewers requirements?

The client libraries in the OpenJDK do as a default rule,
excusing
simple fixes.

>
> I would think that at least for a CSR, which is
OpenJDK-wide, a request
> could be put in with the Skara to track it. A Reviewer
(or Committer?)
> could invoke a /csr command which will require the PR
author to provide a
> link to the CSR, and check that it was approved as part
of the patch
> approval process.

I think that if there is a CSR sub-task in JBS skara can
key off whether
that is approved.
This does mean skara needs to probe JBS but SFAIK its doing
that a
hundred times
a minute anyway.

-phil.

>
> Not sure how complicated it would be to implement.
>
> - Nir
>
> On Mon, Dec 16, 2019 at 5:39 PM Kevin
Rushforthmailto:kevin.rushfo...@oracle.com>>
> wrote:
>
>> That's a good question about whether we can ask for a
slight rewording
>> of the Skara bot message to indicate that there might be
other things to
>> check before "/integrate". I'll raise that question with
the Skara team.
>>
>> As an aside, I noticed the other day that you typed
"/integrate" after a
>> single review, but in that case, there was no clear
indication from Ajit
>> (the first reviewer and the sponsor) that a second
review was required,
>> so I didn't comment on it.
>>
>> -- Kevin
>>
>>
>> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
>>> Kevin,
>>>
>>> thanks for the clarification :) My bad that I didn't
re-read the
>>> contrib.md. But then, who does? The lazy like myself do it
>>> occasionally only (down to once and then forget about
it)
>>>
>>> Maybe the bot message can be improved? With some
indication that its
>>> (the bot's) knowledge about review requirements is
limited, so would
>>> require a careful check by the contributor before
actually post the
>>> /integrate comment? Actually, I think I goofed the
other 

Re: [Rev 03] RFR: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

[Guru Hb]

On Fri, 17 Jan 2020 23:18:28 GMT, Kevin Rushforth  wrote:

>> Previous commits in this pull request have been removed, probably due to a 
>> force push. The incremental views will show differences compared to the 
>> previous content of the PR.
> 
> Looks good to me.
> 
> @guruhb can you re-review this when you get a chance?

+1, Looks good to me.

-

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

Re: [Rev 03] RFR: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

[Guru Hb]

On Fri, 24 Jan 2020 14:41:11 GMT, Robert Lichtenberger  
wrote:

>> As documented in JDK-8236912, WebView did not check whether the idMap really 
>> contained a mapping for the given button, making it prone to errors, when 
>> things are extended (as has happened here).
>> 
>> The fix consists of two test cases that show the problem in unfixed WebViews 
>> and a fix which works analogously to the check whether the given event type 
>> is mapped.
> 
> Previous commits in this pull request have been removed, probably due to a 
> force push. The incremental views will show differences compared to the 
> previous content of the PR.

Marked as reviewed by ghb (Reviewer).

-

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

Re: [Integrated] RFR: 8237823: Mark TextTest.testTabSize as unstable

[Kevin Rushforth]

Changeset: da99e248
Author:Kevin Rushforth 
Date:  2020-01-24 16:54:57 +
URL:   https://git.openjdk.java.net/jfx/commit/da99e248

8237823: Mark TextTest.testTabSize as unstable

Reviewed-by: prr

! modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextTest.java

RFR: 8237833: Check glyph size before adding to glyph texture cache

[Phil Race]

Check if the glyph will fit before trying to cache it.

-

Commits:
 - fbfb2473: 8237833: Check glyph size before adding to glyph texture cache

Changes: https://git.openjdk.java.net/jfx/pull/96/files
 Webrev: https://webrevs.openjdk.java.net/jfx/96/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8237833
  Stats: 9 lines in 1 file changed: 7 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/96.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/96/head:pull/96

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

Re: RFR: 8237823: Mark TextTest.testTabSize as unstable

[Kevin Rushforth]

On Fri, 24 Jan 2020 15:12:50 GMT, Kevin Rushforth  wrote:

> Fix for [JDK-8237823](https://bugs.openjdk.java.net/browse/JDK-8237823).
> 
> The javafx.graphics unit test `TextTest.testTabSize` fails intermittently -- 
> see [JDK-8236728](https://bugs.openjdk.java.net/browse/JDK-8236728).
> 
> This PR marks `TextTest.testTabSize` as unstable, meaning it will not be run 
> by default. It will be run only when running gradle with the 
> `-PUNSTABLE_TEST=true` flag.
> 
> As noted in the bug report, I see this about 20% of the time in our nightly 
> and CI builds. Given that we are getting late in the cycle for openjfx14, we 
> need to mark the test as unstable until a fix can be found.
> 
> NOTE: this is targeted to jfx14.

@prrace can you review this?

-

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

"Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

Just filed a RFE with the following information:

  * Component:
  o JavaFX

  * Subcomponent:
  o fxml: JavaFX FXML

  * Synopsis:
  o "FXMLLoader: if script engines implement javax.script.Compilabel 
compile scripts"

  * Descriptions:
  o "FXMLLoader is able to execute scripts in Java script languages 
(javax.script.ScriptEngine
implementations) if such a Java script language gets defined as the 
controller language in
the FXML file.

If a script engine implements the javax.script.Compilable interface, 
then such scripts could
be compiled and the resulting javax.script.CompiledScript could be 
executed instead using
its eval() methods.

Evaluating the CompiledScript objects may help speed up the execution 
of script invocations,
especially for scripts defined for event attributes in FXML elements 
(e.g. like onMouseMove)
which may be repetitevly invoked and evaluated."

  * System /OS/Java Runtime Information:
  o "All systems."

Received the internal review ID: "9063426"

---rony

Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Kevin Rushforth]

Thank you for filing this enhancement request. As an enhancement it 
should be discussed on this list before proceeding with a pull request 
(although a "WIP" or Draft PR can be used to illustrate the concept).


For my part, this seems like a reasonable enhancement, as long as there 
are no compatibility issues, but it would be good to hear from 
application developers who heavily use FXML.


-- Kevin


On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:

Just filed a RFE with the following information:

   * Component:
   o JavaFX

   * Subcomponent:
   o fxml: JavaFX FXML

   * Synopsis:
   o "FXMLLoader: if script engines implement javax.script.Compilabel compile 
scripts"

   * Descriptions:
   o "FXMLLoader is able to execute scripts in Java script languages 
(javax.script.ScriptEngine
 implementations) if such a Java script language gets defined as the 
controller language in
 the FXML file.

 If a script engine implements the javax.script.Compilable interface, 
then such scripts could
 be compiled and the resulting javax.script.CompiledScript could be 
executed instead using
 its eval() methods.

 Evaluating the CompiledScript objects may help speed up the execution 
of script invocations,
 especially for scripts defined for event attributes in FXML elements 
(e.g. like onMouseMove)
 which may be repetitevly invoked and evaluated."

   * System /OS/Java Runtime Information:
   o "All systems."

Received the internal review ID: "9063426"

---rony

Re: "Internal review ID : 9062887" (Re: FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Kevin Rushforth]

Hi Rony,

This bug was transferred to the JDK project on 28-Nov-2019. I don't know 
why you didn't get an email at that time, but I will inquire of the team 
who processes incoming bugs.


Also, I'll keep an eye out for the RFE you filed today, and let you know 
when it is transferred in case there is still a problem with the 
notification.


-- Kevin


On 1/22/2020 9:52 AM, Rony G. Flatscher wrote:

Hi Anthony,

On 22.01.2020 17:07, Anthony Vanelverdinghe wrote:

Your issue has been converted into a JDK issue, with your testcase attached [1].

Thank you *very* much for this information!


Normally you should’ve received an e-mail at the time of this conversion,

Just searched all my e-mail folders and could not find it (looking for 
"FXMLLoader" in the subject
of e-mails as the bug title contains that word) but could not find a matching 
e-mail for whatever
reasons.


but you can check this yourself by using the internal review ID as in [2]. If 
you’d like to
contribute a fix, see [3].

  


Kind regards, Anthony

  


[1] https://bugs.openjdk.java.net/browse/JDK-8234959


[2] https://bugs.openjdk.java.net/browse/JI-9062887 


[3] https://github.com/openjdk/jfx 


Thank you also for these links (and I learned something new on how to check for 
it using the
internal review id with your [2], thanks a lot for this hint as well)!

Will go back and study all the necessary procedures (forgot a lot since reading 
them the last time)
and will try to contribute the fix in the proper way but it may take me a 
little while (currently
quite busy around here).

---

Maybe one more question: there would be an optimization possible by compiling 
scripts for script
engines that have the javax.script.Compilable interface implemented and use the 
compiled version to
execute/evaluate the scripts (may be helpful for event handler code e.g. for 
onMouseMove event
handlers). Can the fix include such an optimization or should there be a 
separate discussion/RFE for
it beforehand? (Adding this would be trivial in the context of the fix, however 
the bug description
would not hint at such an optimization.)

---rony

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

[Nir Lisker]

On Fri, 24 Jan 2020 16:26:38 GMT, Frederic Thevenet 
 wrote:

>> Here are the results when running JavaFX 14-ea+7. 
>> The columns of the table correspond the width of the target snapshot, while 
>> the rows correspond to the height and the content of the cells is the 
>> average time* spent (in ms) in `Node::snapshot`
>> (*) each test is ran 10 times and the elapsed time is averaged after pruning 
>> outliers, using Grubbs' test.
>> 
>> || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |
>> |---|---|---|---|---|---|---|---|---|
>> | 1024 | 6.304272 | 10.303935 | 15.052336 | 35.929304 | 23.860095 | 
>> 28.828812 | 35.315288 | 27.867205 |
>> | 2048 | 11.544367 | 21.156326 | 28.368750 | 28.887164 | 47.134738 | 
>> 54.354708 | 55.480251 | 56.722649 |
>> | 3072 | 15.503187 | 30.215269 | 41.304645 | 39.789648 | 82.255091 | 
>> 82.576379 | 96.618722 | 106.586547 |
>> | 4096 | 20.928336 | 38.768648 | 64.255423 | 52.608217 | 101.797347 | 
>> 132.516816 | 158.525192 | 166.872889 |
>> | 5120 | 28.693431 | 67.275306 | 68.090280 | 76.208412 | 133.974510 | 
>> 157.120373 | 182.329784 | 210.069066 |
>> | 6144 | 29.972591 | 54.751002 | 88.171906 | 104.489291 | 147.788597 | 
>> 185.185643 | 213.562819 | 228.643761 |
>> | 7168 | 33.668398 | 63.088490 | 98.756212 | 130.502678 | 196.367121 | 
>> 225.166481 | 239.328794 | 260.162501 |
>> | 8192 | 40.961901 | 87.067460 | 128.230351 | 178.127225 | 198.479068 | 
>> 225.806211 | 266.170239 | 325.967840 |
> 
> And here are the results with the change in this PR, on the same machine 
> under Windows 10:
> 
> || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |
> |---|---|---|---|---|---|---|---|---|
> | 1024 | 6.957774 | 10.461498 | 14.721024 | 19.279638 | 47.508266 | 56.585089 
> | 64.522117 | 53.448326 |
> | 2048 | 10.990480 | 19.284461 | 28.235822 | 41.067555 | 92.818088 | 
> 106.782334 | 120.907050 | 112.852554 |
> | 3072 | 15.642648 | 28.502151 | 59.541998 | 55.663658 | 130.654226 | 
> 149.805330 | 205.212356 | 169.002232 |
> | 4096 | 19.882252 | 41.287722 | 59.493687 | 73.809264 | 169.212467 | 
> 200.212097 | 247.934609 | 246.412543 |
> | 5120 | 49.986204 | 97.986781 | 126.127089 | 167.274104 | 217.063815 | 
> 276.812929 | 307.276073 | 366.800463 |
> | 6144 | 66.546156 | 104.339809 | 150.171765 | 206.282107 | 272.486419 | 
> 321.178983 | 365.822047 | 410.087087 |
> | 7168 | 66.894654 | 119.510866 | 176.002883 | 248.937222 | 314.721516 | 
> 380.834398 | 430.858648 | 482.499047 |
> | 8192 | 67.040207 | 112.831977 | 161.852173 | 237.588749 | 319.667719 | 
> 382.151556 | 437.810832 | 451.865266 |

> Here are the results when running JavaFX 14-ea+7.
> The columns of the table correspond the width of the target snapshot, while 
> the rows correspond to the height and the content of the cells is the average 
> time* spent (in ms) in `Node::snapshot`
> (*) each test is ran 10 times and the elapsed time is averaged after pruning 
> outliers, using Grubbs' test.
> 
> 1024  2048307240965120614471688192
> 1024  6.30427210.303935   15.052336   35.929304   
> 23.860095   28.828812   35.315288   27.867205
> 2048  11.544367   21.156326   28.368750   28.887164   
> 47.134738   54.354708   55.480251   56.722649
> 3072  15.503187   30.215269   41.304645   39.789648   
> 82.255091   82.576379   96.618722   106.586547
> 4096  20.928336   38.768648   64.255423   52.608217   
> 101.797347  132.516816  158.525192  166.872889
> 5120  28.693431   67.275306   68.090280   76.208412   
> 133.974510  157.120373  182.329784  210.069066
> 6144  29.972591   54.751002   88.171906   104.489291  
> 147.788597  185.185643  213.562819  228.643761
> 7168  33.668398   63.088490   98.756212   130.502678  
> 196.367121  225.166481  239.328794  260.162501
> 8192  40.961901   87.067460   128.230351  178.127225  
> 198.479068  225.806211  266.170239  325.967840

Any idea why 4096x1024 and 1024x4096 are so different? Same for 8192x1024 and 
1024x8192.

-

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

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

[Frederic Thevenet]

On Fri, 24 Jan 2020 16:03:39 GMT, Frederic Thevenet 
 wrote:

>> I've put together a small benchmark to measure the time it takes to 
>> snapshots into images of sizes varying from 1024*1024 to 8192*8192, by 
>> increment of 1024 in each dimension, which you can find here: 
>> https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java
> 
> Here are the results when running JavaFX 14-ea+7. 
> The columns of the table correspond the width of the target snapshot, while 
> the rows correspond to the height and the content of the cells is the average 
> time* spent (in ms) in `Node::snapshot`
> (*) each test is ran 10 times and the elapsed time is averaged after pruning 
> outliers, using Grubbs' test.
> 
> || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |
> |---|---|---|---|---|---|---|---|---|
> | 1024 | 6.304272 | 10.303935 | 15.052336 | 35.929304 | 23.860095 | 28.828812 
> | 35.315288 | 27.867205 |
> | 2048 | 11.544367 | 21.156326 | 28.368750 | 28.887164 | 47.134738 | 
> 54.354708 | 55.480251 | 56.722649 |
> | 3072 | 15.503187 | 30.215269 | 41.304645 | 39.789648 | 82.255091 | 
> 82.576379 | 96.618722 | 106.586547 |
> | 4096 | 20.928336 | 38.768648 | 64.255423 | 52.608217 | 101.797347 | 
> 132.516816 | 158.525192 | 166.872889 |
> | 5120 | 28.693431 | 67.275306 | 68.090280 | 76.208412 | 133.974510 | 
> 157.120373 | 182.329784 | 210.069066 |
> | 6144 | 29.972591 | 54.751002 | 88.171906 | 104.489291 | 147.788597 | 
> 185.185643 | 213.562819 | 228.643761 |
> | 7168 | 33.668398 | 63.088490 | 98.756212 | 130.502678 | 196.367121 | 
> 225.166481 | 239.328794 | 260.162501 |
> | 8192 | 40.961901 | 87.067460 | 128.230351 | 178.127225 | 198.479068 | 
> 225.806211 | 266.170239 | 325.967840 |

And here are the results with the change in this PR, on the same machine under 
Windows 10:

|| 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |
|---|---|---|---|---|---|---|---|---|
| 1024 | 6.957774 | 10.461498 | 14.721024 | 19.279638 | 47.508266 | 56.585089 | 
64.522117 | 53.448326 |
| 2048 | 10.990480 | 19.284461 | 28.235822 | 41.067555 | 92.818088 | 106.782334 
| 120.907050 | 112.852554 |
| 3072 | 15.642648 | 28.502151 | 59.541998 | 55.663658 | 130.654226 | 
149.805330 | 205.212356 | 169.002232 |
| 4096 | 19.882252 | 41.287722 | 59.493687 | 73.809264 | 169.212467 | 
200.212097 | 247.934609 | 246.412543 |
| 5120 | 49.986204 | 97.986781 | 126.127089 | 167.274104 | 217.063815 | 
276.812929 | 307.276073 | 366.800463 |
| 6144 | 66.546156 | 104.339809 | 150.171765 | 206.282107 | 272.486419 | 
321.178983 | 365.822047 | 410.087087 |
| 7168 | 66.894654 | 119.510866 | 176.002883 | 248.937222 | 314.721516 | 
380.834398 | 430.858648 | 482.499047 |
| 8192 | 67.040207 | 112.831977 | 161.852173 | 237.588749 | 319.667719 | 
382.151556 | 437.810832 | 451.865266 |

-

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

Re: [Integrated] RFR: 8236753: Animations do not play backwards after being stopped

[Nir Lisker]

Changeset: 9ae37f1f
Author:Nir Lisker 
Date:  2020-01-22 20:58:12 +
URL:   https://git.openjdk.java.net/jfx/commit/9ae37f1f

8236753: Animations do not play backwards after being stopped

Reviewed-by: kcr, arapte

! modules/javafx.graphics/src/main/java/javafx/animation/Animation.java
! modules/javafx.graphics/src/test/java/test/javafx/animation/AnimationTest.java
! 
modules/javafx.graphics/src/test/java/test/javafx/animation/SequentialTransitionPlayTest.java

RFR: 8237782: Only read advances up to the minimum of the numHorMetrics or…

[Phil Race]

… the available font data.

-

Commits:
 - 059ec788: 8237782: Only read advances up to the minimum of the numHorMetrics 
or the available font data.

Changes: https://git.openjdk.java.net/jfx/pull/95/files
 Webrev: https://webrevs.openjdk.java.net/jfx/95/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8237782
  Stats: 18 lines in 1 file changed: 18 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/95.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/95/head:pull/95

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

Re: [Rev 01] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag

[Kevin Rushforth]

On Fri, 24 Jan 2020 15:43:25 GMT, Robert Lichtenberger  
wrote:

>> Test simulates a single mouse-released event.
>> Fix simply guards against the null case.
> 
> The pull request has been updated with 1 additional commit.

This is a simple enough change that 1 reviewer will suffice -- @arapte will 
review.

I did notice some unrelated changes that should be reverted.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
 line 1957:

> 1956: }
> 1957: @Override
> 1958: protected void interpolate(double frac) {

This is unrelated to your change. Please revert it.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
 line 1974:

> 1973: }
> 1974: @Override
> 1975: protected void interpolate(double frac) {

Please revert.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
 line 1994:

> 1993: private ListChangeListener childListener = new 
> ListChangeListener() {
> 1994: @Override
> 1995: public void onChanged(Change change) {

Please revert.

-



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

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

[Frederic Thevenet]

On Tue, 21 Jan 2020 21:53:29 GMT, Kevin Rushforth  wrote:

>>> 
>>> 
>>> Looks good to me.
>>> Below is just an observation about time taken by the API,
>>> Platform: Windows10, `maxTextureSize`: 4096
>>> For a snapshot of (4096 * n, 4096 * n): each call to `doSnapshotTile()` 
>>> takes ~100 ms, and each call to `setPixels()` takes ~30 ms.
>>> 
>>> Please wait for one more approval before integrating.
>> 
>> Do you have the same kind of measurements for similar uses cases without 
>> this patch? I'm expecting performances to remain the same for snapshots less 
>> than `maxTextureSize*maxTextureSize` in size, since there is no extra pixel 
>> copy in that case, and the rest of the code if globally unchanged.
>> 
>> Still there exists a window for performance regressions, which for instance 
>> on Windows 10 w/ the D3D rendering pipeline would be when one of the 
>> dimension of a snapshot is >4096  and <8192: in that case a snapshot would 
>> require up to 4 extra copy pixels steps with this patch.
>> This is due to the fact that the old code would accept to render in a 
>> texture of a size up to the non-clamped max texture size (8192 in D3D, 16384 
>> in ES2), but because I couldn't find a clean way to access the non clamped 
>> maxTextureSize exposed by the render factory from the Scene class, I settled 
>> for Prsim.maxTextureSize, which is the clamped value (4096 by default).
>> I haven't dug deep enough in the code to understand precisely why the max 
>> texture size is clamped to 4096 by default, but assuming that it is for a 
>> good reason wouldn't that also apply in that case? Or is it always safe to 
>> use the non-clamped value in that particular case?
> 
> I haven't done any testing yet, but I have two comments on the patch:
> 
> 1. Using the clamped texture size as the upper limit is the right thing to 
> do, but `Prism.maxTexture` isn't guaranteed to be that size. The 
> `Prism.maxTexture` constant represents the value to clamp the texture size 
> to. In the event that D3D or OpenGL were to report a maximum less than the 
> value of `Prism.maxTexture` (unlikely, but maybe on some low-end embedded 
> systems?), then that value is what should be used. The way to get the clamped 
> texture size is to call 
> [`ResourceFactory::getMaximumTextureSize`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/prism/ResourceFactory.java#L222),
>  but you can't do that directly from the scene graph code.
> 
> 2. Allocating multiple `WritableImage` objects and using 
> `PixelWriter::setPixels` to stitch them together will take more temporary 
> memory, and is likely to be slower, than having the snapshot code write into 
> a subimage of the larger allocated image directly.
> 
> Having said that, the current proposed solution is still better than the 
> current behavior is almost all cases, although there could be a performance 
> hit in the case of an 8K x 8K image, which will now be tiled. I want to do a 
> more careful review and some testing. If any other users of snapshot have any 
> comments or concerns, they would be welcome.
> 
> I think the best long-term solution might be to modify the 
> [`QuantumToolkit::renderToImage`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java#L1490)
>  method, although that would certainly be out of scope for JavaFX 14.

I've put together a small benchmark to measure the time it takes to snapshots 
into images of sizes varying from 1024*1024 to 8192*8192, by increment of 1024 
in each dimension, which you can find here: 
https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java

-

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

RFR: 8237823: Mark TextTest.testTabSize as unstable

[Kevin Rushforth]

Fix for [JDK-8237823](https://bugs.openjdk.java.net/browse/JDK-8237823).

The javafx.graphics unit test `TextTest.testTabSize` fails intermittently -- 
see [JDK-8236728](https://bugs.openjdk.java.net/browse/JDK-8236728).

This PR marks `TextTest.testTabSize` as unstable, meaning it will not be run by 
default. It will be run only when running gradle with the 
`-PUNSTABLE_TEST=true` flag.

As noted in the bug report, I see this about 20% of the time in our nightly and 
CI builds. Given that we are getting late in the cycle for openjfx14, we need 
to mark the test as unstable until a fix can be found.

NOTE: this is targeted to jfx14.

-

Commits:
 - bfca349c: 8237823: Mark TextTest.testTabSize as unstable

Changes: https://git.openjdk.java.net/jfx/pull/94/files
 Webrev: https://webrevs.openjdk.java.net/jfx/94/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8237823
  Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/94.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/94/head:pull/94

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

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

[Frederic Thevenet]

On Fri, 24 Jan 2020 16:34:29 GMT, Nir Lisker  wrote:

>> And here are the results with the change in this PR, on the same machine 
>> under Windows 10:
>> 
>> || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |
>> |---|---|---|---|---|---|---|---|---|
>> | 1024 | 6.957774 | 10.461498 | 14.721024 | 19.279638 | 47.508266 | 
>> 56.585089 | 64.522117 | 53.448326 |
>> | 2048 | 10.990480 | 19.284461 | 28.235822 | 41.067555 | 92.818088 | 
>> 106.782334 | 120.907050 | 112.852554 |
>> | 3072 | 15.642648 | 28.502151 | 59.541998 | 55.663658 | 130.654226 | 
>> 149.805330 | 205.212356 | 169.002232 |
>> | 4096 | 19.882252 | 41.287722 | 59.493687 | 73.809264 | 169.212467 | 
>> 200.212097 | 247.934609 | 246.412543 |
>> | 5120 | 49.986204 | 97.986781 | 126.127089 | 167.274104 | 217.063815 | 
>> 276.812929 | 307.276073 | 366.800463 |
>> | 6144 | 66.546156 | 104.339809 | 150.171765 | 206.282107 | 272.486419 | 
>> 321.178983 | 365.822047 | 410.087087 |
>> | 7168 | 66.894654 | 119.510866 | 176.002883 | 248.937222 | 314.721516 | 
>> 380.834398 | 430.858648 | 482.499047 |
>> | 8192 | 67.040207 | 112.831977 | 161.852173 | 237.588749 | 319.667719 | 
>> 382.151556 | 437.810832 | 451.865266 |
> 
>> Here are the results when running JavaFX 14-ea+7.
>> The columns of the table correspond the width of the target snapshot, while 
>> the rows correspond to the height and the content of the cells is the 
>> average time* spent (in ms) in `Node::snapshot`
>> (*) each test is ran 10 times and the elapsed time is averaged after pruning 
>> outliers, using Grubbs' test.
>> 
>> 1024 2048307240965120614471688192
>> 1024 6.30427210.303935   15.052336   35.929304   
>> 23.860095   28.828812   35.315288   27.867205
>> 2048 11.544367   21.156326   28.368750   28.887164   
>> 47.134738   54.354708   55.480251   56.722649
>> 3072 15.503187   30.215269   41.304645   39.789648   
>> 82.255091   82.576379   96.618722   106.586547
>> 4096 20.928336   38.768648   64.255423   52.608217   
>> 101.797347  132.516816  158.525192  166.872889
>> 5120 28.693431   67.275306   68.090280   76.208412   
>> 133.974510  157.120373  182.329784  210.069066
>> 6144 29.972591   54.751002   88.171906   104.489291  
>> 147.788597  185.185643  213.562819  228.643761
>> 7168 33.668398   63.088490   98.756212   130.502678  
>> 196.367121  225.166481  239.328794  260.162501
>> 8192 40.961901   87.067460   128.230351  178.127225  
>> 198.479068  225.806211  266.170239  325.967840
> 
> Any idea why 4096x1024 and 1024x4096 are so different? Same for 8192x1024 and 
> 1024x8192.

I don't, to be honest. 
The results for some dimensions  (not always the same) can vary pretty widely 
from one run to another, despite all my effort to repeat results and remove 
outliers.
Out of curiosity, I also tried to eliminate the GC as possible culprit by 
running it with epsilon, but it seems to make a significant difference.
I ran that test on a laptop with Integrated Intel graphics and no dedicated 
vram (Intel UHD Graphics 620), though, so this might be why. 
Maybe someone could try and run the bench on hardware with a discreet GPU?

-

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

Re: [Integrated] RFR: 8237823: Mark TextTest.testTabSize as unstable

[Kevin Rushforth]

Changeset: da99e248
Author:Kevin Rushforth 
Date:  2020-01-24 16:54:57 +
URL:   https://git.openjdk.java.net/jfx/commit/da99e248

8237823: Mark TextTest.testTabSize as unstable

Reviewed-by: prr

! modules/javafx.graphics/src/test/java/test/javafx/scene/text/TextTest.java

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

[Frederic Thevenet]

On Fri, 24 Jan 2020 15:55:50 GMT, Frederic Thevenet 
 wrote:

>> I haven't done any testing yet, but I have two comments on the patch:
>> 
>> 1. Using the clamped texture size as the upper limit is the right thing to 
>> do, but `Prism.maxTexture` isn't guaranteed to be that size. The 
>> `Prism.maxTexture` constant represents the value to clamp the texture size 
>> to. In the event that D3D or OpenGL were to report a maximum less than the 
>> value of `Prism.maxTexture` (unlikely, but maybe on some low-end embedded 
>> systems?), then that value is what should be used. The way to get the 
>> clamped texture size is to call 
>> [`ResourceFactory::getMaximumTextureSize`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/prism/ResourceFactory.java#L222),
>>  but you can't do that directly from the scene graph code.
>> 
>> 2. Allocating multiple `WritableImage` objects and using 
>> `PixelWriter::setPixels` to stitch them together will take more temporary 
>> memory, and is likely to be slower, than having the snapshot code write into 
>> a subimage of the larger allocated image directly.
>> 
>> Having said that, the current proposed solution is still better than the 
>> current behavior is almost all cases, although there could be a performance 
>> hit in the case of an 8K x 8K image, which will now be tiled. I want to do a 
>> more careful review and some testing. If any other users of snapshot have 
>> any comments or concerns, they would be welcome.
>> 
>> I think the best long-term solution might be to modify the 
>> [`QuantumToolkit::renderToImage`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java#L1490)
>>  method, although that would certainly be out of scope for JavaFX 14.
> 
> I've put together a small benchmark to measure the time it takes to snapshots 
> into images of sizes varying from 1024*1024 to 8192*8192, by increment of 
> 1024 in each dimension, which you can find here: 
> https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java

Here are the results when running JavaFX 14-ea+7. 
The columns of the table correspond the width of the target snapshot, while the 
rows correspond to the height and the content of the cells is the average time* 
spent (in ms) in `Node::snapshot`
(*) each test is ran 10 times and the elapsed time is averaged after pruning 
outliers, using Grubbs' test.

|| 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |
|---|---|---|---|---|---|---|---|---|
| 1024 | 6.304272 | 10.303935 | 15.052336 | 35.929304 | 23.860095 | 28.828812 | 
35.315288 | 27.867205 |
| 2048 | 11.544367 | 21.156326 | 28.368750 | 28.887164 | 47.134738 | 54.354708 
| 55.480251 | 56.722649 |
| 3072 | 15.503187 | 30.215269 | 41.304645 | 39.789648 | 82.255091 | 82.576379 
| 96.618722 | 106.586547 |
| 4096 | 20.928336 | 38.768648 | 64.255423 | 52.608217 | 101.797347 | 
132.516816 | 158.525192 | 166.872889 |
| 5120 | 28.693431 | 67.275306 | 68.090280 | 76.208412 | 133.974510 | 
157.120373 | 182.329784 | 210.069066 |
| 6144 | 29.972591 | 54.751002 | 88.171906 | 104.489291 | 147.788597 | 
185.185643 | 213.562819 | 228.643761 |
| 7168 | 33.668398 | 63.088490 | 98.756212 | 130.502678 | 196.367121 | 
225.166481 | 239.328794 | 260.162501 |
| 8192 | 40.961901 | 87.067460 | 128.230351 | 178.127225 | 198.479068 | 
225.806211 | 266.170239 | 325.967840 |

-

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

Re: RFR: 8237823: Mark TextTest.testTabSize as unstable

[Phil Race]

On Fri, 24 Jan 2020 15:12:50 GMT, Kevin Rushforth  wrote:

> Fix for [JDK-8237823](https://bugs.openjdk.java.net/browse/JDK-8237823).
> 
> The javafx.graphics unit test `TextTest.testTabSize` fails intermittently -- 
> see [JDK-8236728](https://bugs.openjdk.java.net/browse/JDK-8236728).
> 
> This PR marks `TextTest.testTabSize` as unstable, meaning it will not be run 
> by default. It will be run only when running gradle with the 
> `-PUNSTABLE_TEST=true` flag.
> 
> As noted in the bug report, I see this about 20% of the time in our nightly 
> and CI builds. Given that we are getting late in the cycle for openjfx14, we 
> need to mark the test as unstable until a fix can be found.
> 
> NOTE: this is targeted to jfx14.

Marked as reviewed by prr (Reviewer).

-

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

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

[Frederic Thevenet]

On Fri, 24 Jan 2020 16:55:47 GMT, Frederic Thevenet 
 wrote:

>>> Here are the results when running JavaFX 14-ea+7.
>>> The columns of the table correspond the width of the target snapshot, while 
>>> the rows correspond to the height and the content of the cells is the 
>>> average time* spent (in ms) in `Node::snapshot`
>>> (*) each test is ran 10 times and the elapsed time is averaged after 
>>> pruning outliers, using Grubbs' test.
>>> 
>>> 10242048307240965120614471688192
>>> 10246.30427210.303935   15.052336   35.929304   
>>> 23.860095   28.828812   35.315288   27.867205
>>> 204811.544367   21.156326   28.368750   28.887164   
>>> 47.134738   54.354708   55.480251   56.722649
>>> 307215.503187   30.215269   41.304645   39.789648   
>>> 82.255091   82.576379   96.618722   106.586547
>>> 409620.928336   38.768648   64.255423   52.608217   
>>> 101.797347  132.516816  158.525192  166.872889
>>> 512028.693431   67.275306   68.090280   76.208412   
>>> 133.974510  157.120373  182.329784  210.069066
>>> 614429.972591   54.751002   88.171906   104.489291  
>>> 147.788597  185.185643  213.562819  228.643761
>>> 716833.668398   63.088490   98.756212   130.502678  
>>> 196.367121  225.166481  239.328794  260.162501
>>> 819240.961901   87.067460   128.230351  178.127225  
>>> 198.479068  225.806211  266.170239  325.967840
>> 
>> Any idea why 4096x1024 and 1024x4096 are so different? Same for 8192x1024 
>> and 1024x8192.
> 
> I don't, to be honest. 
> The results for some dimensions  (not always the same) can vary pretty widely 
> from one run to another, despite all my effort to repeat results and remove 
> outliers.
> Out of curiosity, I also tried to eliminate the GC as possible culprit by 
> running it with epsilon, but it seems to make a significant difference.
> I ran that test on a laptop with Integrated Intel graphics and no dedicated 
> vram (Intel UHD Graphics 620), though, so this might be why. 
> Maybe someone could try and run the bench on hardware with a discreet GPU?

With regard to why the tiling version is significantly slower, though, I do 
have a pretty good idea; as Kevin hinted, the pixel copy into a temporary 
buffer before copying into the final image is where most the extra time is 
spent.
The reason why is is some much slower is a little bit of a pity, though; 
profiling a run of the benchmark shows that a lot of time is spent into 
`IntTo4ByteSameConverter::doConvert` and the reason for this turns out that 
this is due to the fact that, under Windows and the D3D pipeline anyway, the 
`WriteableImage` used to collate the tiles and the tiles returned from the 
RTTexture have different pixel formats (IntARGB for the tile and byteBGRA for 
the `WriteableImage`).
So if we could use a `WriteableImage` with an IntARGB pixel format as the 
recipient for the snapshot (at least as long as no image was provided by the 
caller), I suspect that the copy would be much faster.
Unfortunately it seems the only way to choose the pixel format for a 
`WritableImage` is to initialize it with a `PixelBuffer`, but then one can no 
longer use a `PixelWriter` to update it and it desn't seems to me that there is 
a way to safely access the `PixelBuffer` from an image's reference alone.
I'm pretty new to this code base though (which is quite large; I haven't read 
it all quite yet... ;-), so hopefully there's a way to do that that has eluded 
me so far.

-

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

TableView slow vertical scrolling with 300+ columns

[Ed Kennard]

Hi everyone,

I’m new to the list, so by way of a short introduction, I’ve been working with 
JavaFX for the last 4 years developing a commodities trading risk management 
system from the ground up for a software company I co-founded in London.  All 
our code is written in Scala, the functional style of which is essential for 
the mathematical heavy lifting needed on the backend, but which also lends 
itself really well to UI programming and working with JavaFX.  I’m enthusiastic 
about JavaFX and would love to make a contribution to the project.

At the center of our product is an extension of the TableView control that’s 
responsible for displaying all the output from our pivot reporting engine.  
Depending on how the user configures the layout of their pivot reports, 
sometimes there are a legitimately large number of columns (300+).  When that 
happens, while the horizontal scrolling remains perfectly smooth, the vertical 
scrolling degrades to a somewhat juddery state and CPU usage spikes.

I found an issue raised about this in 2019 on the old JFX GitHub repo here…
https://github.com/javafxports/openjdk-jfx/issues/409

…but I’m not sure whether, per Kevin’s suggestion at the bottom, it was ever 
submitted through the correct channels.  I can confirm that the test code 
included there by “yososs” on 20th May 2019 perfectly illustrates the problem 
I’m experiencing.  The same person seems to have a fairly clear theory on what 
is causing the problem, too - see their follow-up comment on 12 Sept 2019.

So, my questions to the list are:


  1.  Has anyone seen this issue raised anywhere else?
  2.  If yes, has anyone taken a look into it yet, and possibly even found a 
fix?
  3.  If no to both of the above, shall I submit it through the correct 
channels then have a crack at fixing myself?  Or is the issue likely to be a 
much deeper and far-reaching one than I’m anticipating?

Many thanks

Ed