On Sun, 24 Jan 2021 09:02:17 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:

>> I left a few inline comments below.
>
> Sorry about the force push, merging over a year of changes from master did 
> not seem right to me.  It was only for getting up to date, I will do the 
> other commits normally.

> 1. Merge in the latest changes from the upstream master branch into your 
> local feature branch? Among other things, this will enable running tests via 
> GitHub Actions.

I hadn't seen that in practice yet, it looks really nice.

> 2. Add a test program under `tests/performance` that can be used to verify 
> the performance gain. Even better would be if you can create an automated 
> test under `tests/system`. I was thinking something like what we did for PR 
> #34 where there was a pathological performance bug fixed and the test checked 
> that the operation in question didn't take more than some small number of 
> seconds. That might be overkill for this fix, though.

I put a test under `tests/system` using the example you gave.  It creates a 
Scene with about 16k nodes, and then does 10.000 operations on them.  This runs 
in around 2-2.5 seconds before the fix and in about 100-200 ms after.  I put 
the cut-off point somewhere in the middle (800 ms).

I am planning to address the rest of the comments somewhere in the coming week.

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

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

Reply via email to