On Fri, 10 Jan 2020 00:34:18 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>>> This will need a fair bit of testing to ensure that there are no >>> regressions either in functionality or (especially) performance, in >>> addition to tests for the new functionality. >> >> Which tests for the new functionality should I write? Up to the shader >> itself it's mostly just passing on variables, and the API is standard >> `DoubleProperty`s. I can test the dirty bits / redraw logic. >> >>> On the performance aspect, the inner loop of the lighting calculation now >>> has an additional if test for the max range and additional arithmetic >>> calculations for the attenuation. What we will need is a test program that >>> we can run on Mac and Windows to measure the performance of rendering in a >>> fill-rate-limited case. Ideally, we would not pay much of a performance hit >>> in the default case where `ca == 1, la == 0, qa == 0`, but we first need to >>> be able to measure the drop in performance before we can say whether it is >>> acceptable. >> >> Can you point me to a similar performance test? I didn't see a way to easily >> measure FPS. >> I don't know how to avoid the `if` test for the `maxRange`, I'm open to >> suggestions. The only thing I can think of is if `maxRange` is infinite (the >> default), we will swap the shader for one that doesn't make that check. >> However, swapping shaders also costs performance. > > We have a few performance tests in apps/performance, but I don't know how up > to date they are. They might give you a starting point on how to measure FPS, > but really the harder part is going to be coming up with a workload -- a > scene with a number of Shape3Ds with large triangles (so that they are > fill-rate limited) and 1-3 lights, etc -- that you can use to measure > rendering performance without measuring overhead. Typically you want a scene > that is rendering continuously in the < 30 fps range, and more like 10 fps to > minimize the overhead even more. > > Before we figure out whether we need to double the number of shaders (which > was one of the ideas that I had as well), we need to know how much of a > problem it is. Is it < 2% performance drop on typical cases on a variety of > machines or it is a 25% drop (or more likely somewhere in between). If the > perf drop is negligible, then it isn't worth doubling the shaders. If it is > significant, then we probably need to. > > If we do need to double the shaders, I wouldn't do it based on the maxRange > being infinite, rather I would do it based on whether attenuation is being > used. That way the existing shaders would be unchanged, while the new shader > would deal with both attenuation and the maxRange test. > > Hopefully, there won't be enough of a perf hit to require doubling the > shaders, but we need to be able to measure it. > > For functional testing, in addition to the simple API tests, we want to make > sure that the basic rendering is working with and without attenuation. Some > sort of visual test where you verify that attenuation is / isn't happening as > well as testing the cutoff. I wouldn't get too fancy with these...just a > sanity test. Looks like the `jcheck` bot removed the `rfr` label because the CSR isn't complete. An incomplete CSR should be treated the same way as an insufficient number of reviewers. I filed [SKARA-262](https://bugs.openjdk.java.net/browse/SKARA-262) to track this. ------------- PR: https://git.openjdk.java.net/jfx/pull/43
