Fish Trophy

chipx86 got a fish trophy!

Improve performance and visuals when resizing the diff viewer window.

Review Request #8888 — Created April 7, 2017 and submitted

Information

Review Board
release-2.5.x
e27eb38...

Reviewers

Resizing the diff viewer was not very smooth, and some UI elements would
stutter. The active chunk highlighter would sort of jump around a little
bit or lag behind when the chunks changed position or height. There was
also a horizontal scrollbar that would flicker on and off when resizing
the window smaller. Along with this, it was just a bit too slow to
resize.

Many of these problems were ordering issues in event processing and
style issues. We also just had some less-than-optimal logic in resize
handlers.

Resize handling is now driven primarily by the diff viewer page,
allowing some coordination to take place. Upon receiving a resize event
(or a layout is otherwise scheduled), the new _.throttleLayout() is
called. This makes use of requestAnimationFrame() to schedule the
function used to compute the new layout. If there are subsequent calls
to this before the new layout has been computed (which can happen during
resizes with very heavy pages), they'll be ignored, keeping us from
swamping the browser.

The layout handler tells each diff to update its layout, and then tells
the chunk highlighter to update. This ensures that any layout changes
from the diffs can be immediately reflected in the highlighter, and
prevents all this from constantly happening while resizing the page
(which caused resizes to be slow and events to accumulate).

The chunk highlighter's been optimized quite a bit. Some jQuery calls,
which are more expensive than needed for this work, have been replaced
with element attribute lookups, and the logic for what CSS to apply and
when has been reworked. The width is no longer set on resize. Instead,
we simply dock to the left and right of the container, allowing the
browser to handle that and preventing the horizontal scrollbars from
appearing.

Resize handlers are also more aware of vertical vs. horizontal window
sizes, preventing any work from occurring if the resize wouldn't result
in any changes.

Now that the orders are guaranteed, we can also remove the defer() for
the chunk highlighter, which allows the highlighter to smoothly appear
bound to the chunk as its position or height changes.

The result is much smoother resizing, improved logic, and simplified
code that's easier to reason about.

Tested on Chrome, Firefox, and Safari. Resizing was noticeably improved
on all browsers, particularly on Firefox and Safari.

Profiled resizing diffs and switching chunks before and after.

Unit tests pass.

Description From Last Updated

I think we should use _.throttle instead of _.debounce. Debounce will wait at least the delay before the first call, …

daviddavid

Same thing here: adding in a mandatory 300ms delay is going to make things perceive as slower for the general …

daviddavid

This can just be done with the variable definition.

daviddavid

requestAnimationFrame is available in all the browsers we support.

daviddavid
david
  1. 
      
  2. I think we should use _.throttle instead of _.debounce. Debounce will wait at least the delay before the first call, whereas throttle will call it immediately.

    How did you come up with 300ms?

    1. I played around a lot with both _.throttle and _.debounce before arriving at this, with small diffs and large diffs. There are tradeoffs, but I'll explain why I went with _.debounce.

      _.throttle will indeed call it immediately, and will continue to call even if we're still getting frequent resize events. This can be just fine in most cases, but with diffs it's more expensive than normal, particularly on Firefox. Even with the other optimizations, we're making some styling changes that force the browser to rerender the entire diff, and for larger (but real-world) diffs this can be slow. (For very large diffs, this can be a death sentence to the browser, but I'm not optimizing for that case).

      With many diffs on the page (such as with a couple installs we know of that increased their max diffs per page), this can quickly swamp the browser, since we're performing the rerenders on each one. What happens is that the resize events start to queue up. Our code will be handling a resize event, setting new styles, and forcing the browser to re-render the page. While the browser's trying to do that, you get another resize event, which gets throttled and then turns into another re-render of the page. And then another, and another. These start to get queued up, since the JavaScript is running but the browser's still trying to render the page. I've managed to crash the tab when viewing 20 large-ish (but, again, real-world) diffs and resizing the window back-and-forth for about 20 seconds. Sure, most people aren't going to do that, but it shows there is a problem. A problem that goes away if using _.debounce.

      Using _.debounce, we avoid this by preventing events from happening unless there's been some time since the last events. It gives the browser more time to deal with the render of the page. It does mean that we don't update the page as frequently if spending a lot of time resizing, but it's far better than having the same effect from swamping the browser. Overall, _.debounce, in this case, prevents a lot of problems.

      The 300ms comes from playing with numbers and larger (but real-world) diff sizes across different browsers. I found that 100-150ms with larger diffs still resulted in a bunch of resize events being queued up in a normal resize of a browser window (resizing at a normal rate, not too fast, not too slow). At 250-300ms, resizes were being processed far less often, but still frequently enough to make the resize happen when the user stopped resizing. I chose 300ms over 250ms to compensate just a bit more for resizing speeds. Out of all the numbers I played with, with various diff sizes and numbers of diffs on the page, this gave me the best results.

      Now, all that being said, I don't love it. I'd love a smooth resize without any delays at all. We have that today if the browser is cooperating. Chrome does pretty well here (unless dealing with huge diffs), but Firefox does not. I do plan to tweak this further, and maybe I'll adjust the numbers or the logic more. I've thought about having the logic adjust based on the size of the diff, and I think there's some interesting things to explore there.

      But anyway, those were the observations I made when stress-testing the browser, and the reason for those choices. I don't consider this the end of this optimization work. Just a step.

    2. I played around with this more, switching away from a _.debounce/_.throttle and moving to requestAnimationFrame (with a flag to essentially do debouncing). I was able to smoothly resize standard diffs, processing the diffs more regularly, and for my stress-test diffs that caused slower rendering, I received the events less often during my resizes. I wasn't able to reproduce the crashed tab case. So this seems like the right approach. I'll post a change probably tomorrow with this.

  3. Same thing here: adding in a mandatory 300ms delay is going to make things perceive as slower for the general case. I think throttle was correct.

  4. This can just be done with the variable definition.

  5. 
      
chipx86
david
  1. 
      
  2. requestAnimationFrame is available in all the browsers we support.

    1. Figured it's a "better safe than sorry" check. IE < 11 (which people no doubt are going to at least try to use) doesn't have it. I guess I don't have a strong opinion here.

    2. According to caniuse.com, IE10 supports it too.

  3. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (429a11e)
Loading...