Improve performance scaling #50

jaxry · 2019-12-06T00:30:19Z

Brief Description

This PR improves the algorithm that determines each draw call's tile size. This ensures the renderer remains responsive while also rendering as quickly as possible.

Pull Request Guidelines

I have added pull requests labels which describe my contribution.
All existing tests passed.
- I have added tests to cover my changes, of which pass.
I have compared the render output of my branch to master.
My code has passed the ESLint configuration for this project.
My change requires modifications to the documentation.
- I have updated the documentation accordingly.

jaxry · 2019-12-06T00:32:34Z

src/RayTracingRenderer.js

@@ -106,17 +104,6 @@ export function RayTracingRenderer(params = {}) {

  module.getPixelRatio = () => pixelRatio;

-  module.setRenderTime = (time) => {


I think we can simply remove this method from the API. It seems there's an ideal render time (~50 fps) which balances responsiveness and render speed that's the same between all the devices I tested on, so there's no reason to change it.

@jaxry which devices have you tested on?

My Macbook with an integrated Intel GPU (low-mid end), and my desktop with the GTX 2080 (very high end) on both Linux and Windows (different drivers, different results). Both devices had the performance issues we've noted, and this PR fixes it for both. I think the most useful device to test at this point would be a low-end one.

Could you briefly explain how this works compared to before? It looks like expAvg is controlling convergence rate, similarly to the old convergenceStrength variable. Why did you choose 0.5?

Based on the current frame's performance, we calculate how long each pixel takes to compute, and from that derive the ideal resolution that takes the desired amount of time to sample (20 ms in this case).

This^ is basically the same thing that was happening before right?

The exponential averaging is not necessary, but what it does is smooth out performance differences between frames. The higher the expAvg, the more stable the fps becomes, but the slower it responds to changes in the scene or camera. 0.5 was a stable number for my hardware that still adjusted the framerate in about 1 second whenever I moved the camera to a more complex angle.

makes sense, how different is this than what was happening before?

When your camera stays still, does your FPS do a good job hovering around 50 fps? If not, this number should be increased.

bounces between 47 and 53

This^ is basically the same thing that was happening before right?

I actually tried this current method months ago, I didn't think to use an exponential average, so the framerate varied too much with this approach. So the method before this PR steered pixelsPerTile to the right number using a smaller, arbitrary step that scaled with the error. That was more stable but it look longer to converge.

But now this current technique is more exact AND stable with the help of averaging.

great, nice going!

It would be great to avoid breaking changes when possible.
Calling this function would now cause a crash.

jaxry · 2019-12-06T00:34:26Z

src/renderer/RenderingPipeline.js

+    const previewHeight = Math.round(clamp(previewWidth / aspectRatio, 1, hdrBuffer.height));
+
+    const diff = Math.abs(previewWidth - hdrPreviewBuffer.width) / previewWidth;
+    if (diff > 0.05) { // don't bother resizing if the buffer size is only slightly different


Resizing the buffer takes time and we can avoid it if the change in buffer size is small. This approach is a definite improvement, but I'm not happy with it all together.

In a separate PR, I will overhaul the buffer system so that the preview buffer actually uses a small viewport of the main buffer, instead of constantly resizing an independent buffer.

jaxry · 2019-12-06T00:35:50Z

src/renderer/TileRender.js

+      const expAvg = 0.5;
+
+      const newPixelsPerTile = pixelsPerTile * desiredTimePerTile / timePerTile;
+      pixelsPerTile = expAvg * pixelsPerTile + (1 - expAvg) * newPixelsPerTile;


Calculating the ideal pixels per tile (to match desiredTimePerTile) in a different way. This method is way faster in both converging to the ideal time, and also remaining stable once it reaches that time.

Could you briefly explain how this works compared to before? It looks like expAvg is controlling convergence rate, similarly to the old convergenceStrength variable. Why did you choose 0.5?

jaxry · 2019-12-06T00:37:27Z

src/renderer/TileRender.js

+  } else if (maxRenderbufferSize === 16384) {
+    return 400000;
+  } else if (maxRenderbufferSize >= 32768) {
+    return 600000;


Significantly increase the initial estimates for all devices. The previous numbers were too low in practice and took a while to converge to the actual numbers for each device.

In the case that these numbers are too high, the new converging method quickly brings them to the right range (only 1 or 2 draw calls needed)

it might be good to add these magic numbers as const variables.

jaxry · 2019-12-06T00:38:28Z

This PR brings a significant improvement for my laptop and desktop. We should test these changes on a few other devices too.

lyonsno · 2019-12-10T21:19:14Z

How low end? Would a 13 inch mbp do the trick?

…

On Tue, Dec 10, 2019 at 1:18 PM Lucas Crane ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/RayTracingRenderer.js <#50 (comment)> : > @@ -106,17 +104,6 @@ export function RayTracingRenderer(params = {}) { module.getPixelRatio = () => pixelRatio; - module.setRenderTime = (time) => { My Macbook with an integrated Intel GPU (mid end), and my desktop with the GTX 2080 (very high end) on both Linux and Windows (different drivers, different results). Both devices had the performance issues we've noted, and this PR fixes it for both. I think the most useful device to test at this point would be a low-end one. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#50?email_source=notifications&email_token=AAFN2ZE673EDYVXJUZ37MVTQYABQVA5CNFSM4JWF6QYKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOW3IBQ#discussion_r356282082>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFN2ZGUBXRCH5WU3YVGICLQYABQVANCNFSM4JWF6QYA> .

jaxry · 2019-12-10T21:21:02Z

I think so. We should pay attention to whether the preview window lags when moving the camera, and whether the renderer remains responsive when the camera stays still.

lyonsno

I'd like to test this on another device before approving, but on my machine this is certainly an improvement! Responsiveness has increased, and convergence speed has also increased!

lyonsno · 2019-12-11T01:05:00Z

src/renderer/RenderingPipeline.js

@@ -80,12 +80,15 @@ export function makeRenderingPipeline({
  }

  function setPreviewBufferDimensions() {
-    const aspectRatio = hdrBuffer.width / hdrBuffer.height;
-    const desiredTimeForPreview = 16; // 60 fps
+    const desiredTimeForPreview = 10;


I notice my preview buffer starts at a lower resolution than before, but it does not adjust upwards. Is this because the lower resolution is all that my computer can handle?

What is your framerate while moving the camera?

56 - 60, settling on 60 after a few seconds. On master, which has a higher res preview buffer, framerate bounces around between 32 and 51. Maybe the buffer on this branch just happens to start with the correct settings for my machine?

The ideal FPS for the preview buffer would be something just below 60 but not actually 60. That would mean we're making max usage of the hardware, while also keeping things responsive. Maybe we can increase this number a little, but in complex scenes it will then be too high according to my testing. That's because we're currently estimating the preview size based on the running tile size. But this estimation isn't exact since it doesn't account for ray coherence across the entire scene (vs just a tile), among other issues.

I think our best bet, in the future, would be to have a separate benchmark that just controls the preview window's size. Do you have other ideas? I'd like to tackle this during the buffer management refactor I'll be tackling in a week or two.

lyonsno · 2019-12-11T01:07:07Z

src/renderer/TileRender.js

@@ -31,31 +31,17 @@ export function makeTileRender(gl) {

  let pixelsPerTileQuantized = pixelsPerTile;

-  let desiredTimePerTile = 22; // 45 fps
+  let desiredTimePerTile = 20;


Did you arrive at this number through the testing you did? If so, should we test on one more even lower spec'd device to make sure it is low enough?

The lower bound for this number is 17ms, which corresponds to 60 fps. At that point the hardware is being underworked since the animation frame caps at 60. 20ms puts it at ~50 fps which still provided responsiveness for the devices I tested, while also being 10 FPS below the limit. That 10 FPS buffer lets the framerate fluctuate around the amount you reported, while also not hitting 60, which would kill the sampling speed if hit.

In any case, testing this number on a lower spec device is important to see if what I'm saying still applies to those devices. But since this number is already lower than it was before this PR, it should run better regardless.

lyonsno · 2019-12-11T01:08:47Z

src/renderer/TileRender.js

+  } else if (maxRenderbufferSize === 16384) {
+    return 400000;
+  } else if (maxRenderbufferSize >= 32768) {
+    return 600000;


lyonsno · 2019-12-11T01:10:03Z

src/renderer/TileRender.js

+      const expAvg = 0.5;
+
+      const newPixelsPerTile = pixelsPerTile * desiredTimePerTile / timePerTile;
+      pixelsPerTile = expAvg * pixelsPerTile + (1 - expAvg) * newPixelsPerTile;


Could you briefly explain how this works compared to before? It looks like expAvg is controlling convergence rate, similarly to the old convergenceStrength variable. Why did you choose 0.5?

jaxry · 2019-12-11T01:27:32Z

Could you briefly explain how this works compared to before? It looks like expAvg is controlling convergence rate, similarly to the old convergenceStrength variable. Why did you choose 0.5?

Based on the current frame's performance, we calculate how long each pixel takes to compute, and from that derive the ideal resolution that takes the desired amount of time to sample (20 ms in this case).

The exponential averaging is not necessary, but what it does is smooth out performance differences between frames. The higher the expAvg, the more stable the fps becomes, but the slower it responds to changes in the scene or camera. 0.5 was a stable number for my hardware that still adjusted the framerate in about 1 second whenever I moved the camera to a more complex angle.

When your camera stays still, does your FPS do a good job hovering around 50 fps? If not, this number should be increased.

lyonsno

Ok device testing is still in progress/slightly blocked, but like you said I don't see how this could possibly be a regression. I vote we merge and we can always make another PR if things could be further improved for low end devices.

Jaxry added 3 commits December 5, 2019 15:52

improve performance

0fc5afd

cleanup

7ff6b07

more cleanup

26f5f9d

jaxry added the bug Something isn't working label Dec 6, 2019

jaxry requested review from carlos-lopez-garces, elfrank, lyonsno and santiagoroca as code owners December 6, 2019 00:30

jaxry commented Dec 6, 2019

View reviewed changes

remove old comment

bcd7e7f

jaxry commented Dec 6, 2019

View reviewed changes

slightly increase preview resolution

69040c2

lyonsno reviewed Dec 11, 2019

View reviewed changes

lyonsno approved these changes Dec 11, 2019

View reviewed changes

Merge branch 'master' into improve-performance-scaling

e9d4f97

jaxry merged commit 71f778f into master Dec 11, 2019

jaxry deleted the improve-performance-scaling branch December 11, 2019 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance scaling #50

Improve performance scaling #50

jaxry commented Dec 6, 2019 •

edited

Loading

jaxry Dec 6, 2019 •

edited

Loading

lyonsno Dec 10, 2019

jaxry Dec 10, 2019 •

edited

Loading

lyonsno Dec 11, 2019

jaxry Dec 11, 2019

lyonsno Dec 11, 2019

elfrank Dec 11, 2019

jaxry Dec 6, 2019 •

edited

Loading

jaxry Dec 6, 2019

lyonsno Dec 11, 2019

jaxry Dec 6, 2019

lyonsno Dec 11, 2019

elfrank Dec 11, 2019

jaxry commented Dec 6, 2019

lyonsno commented Dec 10, 2019 via email

jaxry commented Dec 10, 2019

lyonsno left a comment

lyonsno Dec 11, 2019

jaxry Dec 11, 2019 •

edited

Loading

lyonsno Dec 11, 2019

jaxry Dec 11, 2019

lyonsno Dec 11, 2019

jaxry Dec 11, 2019 •

edited

Loading

lyonsno Dec 11, 2019

lyonsno Dec 11, 2019

jaxry commented Dec 11, 2019

lyonsno left a comment

		@@ -106,17 +104,6 @@ export function RayTracingRenderer(params = {}) {

		module.getPixelRatio = () => pixelRatio;

		module.setRenderTime = (time) => {

Improve performance scaling #50

Improve performance scaling #50

Conversation

jaxry commented Dec 6, 2019 • edited Loading

Brief Description

Pull Request Guidelines

jaxry Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaxry Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaxry Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaxry commented Dec 6, 2019

lyonsno commented Dec 10, 2019 via email

jaxry commented Dec 10, 2019

lyonsno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaxry Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaxry Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaxry commented Dec 11, 2019

lyonsno left a comment

Choose a reason for hiding this comment

jaxry commented Dec 6, 2019 •

edited

Loading

jaxry Dec 6, 2019 •

edited

Loading

jaxry Dec 10, 2019 •

edited

Loading

jaxry Dec 6, 2019 •

edited

Loading

jaxry Dec 11, 2019 •

edited

Loading

jaxry Dec 11, 2019 •

edited

Loading