Reduce lag experienced when expanding doc table rows #9326

Bargs · 2016-12-01T23:54:23Z

There were a couple of issues making row expansion slow in the doc table.

Here's a timeline snapshot before I made any changes:

We see giant forced reflows, courtesy of the ACE editor. ACE is used to display the JSON view of the doc, which we don't even show by default. So commit # 1 prevents rendering the JSON view until the user actually clicks on the JSON tab.

Now the timeline looks like this:

Slightly better, but now fixed_scroll.js is causing more forced reflows. This is because it's calculating DOM element widths during the digest cycle.

The purpose of the fixed scroll directive is to add a horizontal scroll bar to the bottom of the window which proxies scroll events to the target element's scroll bar. This is useful in cases like the doc table, where the real container's scroll bar might be waaay at the bottom of the page (and in fact inaccessible due to infinite scroll loading).

I thought I might be able to give the doc table container a height equal to the remaining space on the page using flexbox, pulling the real horizontal scrollbar up to the bottom of the window. It worked, but when I switched everything to flex the performance was even worse.

So I landed on the solution in commit # 2, replacing the angular watcher with a timeout that checks the element's width at regular intervals. Timeouts are ugly, but the existing code was basically already on a timeout, and this new solution has the benefit of not being tied to the digest cycle where it'll slow down the page during user interactions.

The final timeline:

Total time to handle the click event went from ~700ms to ~150ms.

I'd be open to other solutions, but in the interest of time I figured this was an ok compromise and an improvement over what we already have.

Bargs · 2016-12-02T00:15:59Z

Hmmm, probably a timing issue in the tests, I'll check that out tomorrow.

Bargs · 2016-12-02T17:45:11Z

Tests should be good now

lukasolson · 2016-12-02T21:36:05Z

src/ui/public/fixed_scroll.js

+ * This directive adds a fixed horizontal scrollbar to the bottom of the window that proxies its scroll events
+ * to the target element's real scrollbar. This is useful when the target element's horizontal scrollbar
+ * might be waaaay down the page, like the doc table on Discover.
+ */


Thanks for the clarifying comment. 👍

lukasolson · 2016-12-02T21:42:47Z

src/ui/public/fixed_scroll.js

-      ], setup);
+      let width;
+      let scrollWidth;
+      let widthCheckTimeout = $timeout(checkWidth, 500);


Hmm, is there anything we can listen to (like window resize) to invoke this, or do we really have to do it every half-second?

I thought about window resize, but there are other situations where the container width might change, like adding/removing columns from the doc table. I also tried the scroll event, thinking it might fire when the container changes size since the scroll bar would also be changing size, but no dice :(

afaik there is no event that'll tell us when the container has changed size 100% of the time

On the plus side, as long as the content of the page hasn't changed the timeout function is pretty lightweight. Definitely still open to other ideas though.

lukasolson · 2016-12-08T18:57:13Z

Blah, I really like the idea of just removing the fixedScroll directive altogether. I really don't see its utility. You can scroll horizontally without having to use the scroll bar (using a trackpad, or shift+mouse wheel). I think the only reason I ever want to use the scroll bar is when I need to scroll a really far distance quickly, and that isn't a valid use case here. I think this code adds unnecessary complexity and performance issues, and sometimes it behaves kinda unexpectedly. I'm definitely +1 on removing this code altogether. Thoughts @Bargs @weltenwort @spalger @epixa?

Bargs · 2016-12-08T20:10:19Z

I'd really love to just rip it out, but I do worry about less savvy users not knowing about shift+scroll :/

weltenwort · 2016-12-09T09:21:52Z

Are there any situations where we actually want to horizontal scrolling to happen? Maybe it would be more beneficial to invest some effort into avoiding that in general (e.g. by performing more wrapping, adjusting styles via media queries, etc...)?

Bargs · 2016-12-10T00:11:35Z

I tried to come up with some creative ideas today but nothing really stuck. I also played around with flexbox some more but to no avail. I wish CSS grid was available in browsers already.

Open to suggestions.

weltenwort · 2016-12-12T10:12:39Z

So if my understanding is correct, the fixedScroll thing is needed because we perform horizontal scrolling on the doctable container but vertical scrolling on the body. Using flexbox you attempted to move the vertical scrolling to the doctable container as well?

Doing that sounds very reasonable and might be a good idea for all columns in the discover view. I'm always annoyed when the field selector column or even the time picker scroll out at the top.

I wouldn't be surprised if the bad flexbox performance stems from the inefficient dom manipulation that is being done in some places inside the doc table or from unclean javascript code that triggers reflows/layouts by performing measurements. Maybe css tables (display: table) would be worth investigating while we're waiting for grid layouts?

Bargs · 2016-12-12T15:51:06Z

It's certainly possible that our code or markup is interacting poorly with flexbox, but I think getting it to work well, if possible, is high fruit. It also has some other issues as well. Getting the doc table container to fill the height of its parent without growing to the size of its content requires specific styles be applied up the DOM hierarchy all the way to the body. The more I play with it the more I feel flexbox isn't good for such high level layout.

I don't think tables will help us here because there's no way to get a table row to fill its container without growing with the size of its content, which is what we need the doc table to do so that it fills all the vertical space in the window while remaining scrollable if the content is larger than the window. Let me know if I'm misunderstanding what you meant though.

As it stands, I think this PR improves performance without making fixedScroll any worse than it already is. If you guys agree, I'd like to get this merged and discuss larger rewrites of the doc table as a separate issue. It sounds like we all would like to see it cleaned up, but I think that work needs to stand on its own merits because this small issue doesn't warrant such an effort.

Bargs · 2016-12-12T23:15:37Z

I just noticed this PR while reading my github notifications. I'm gonna test it tomorrow to see if I can use it to get rid of the timeout.

Bargs · 2016-12-15T21:58:05Z

@lukasolson @weltenwort turns out ResizeObserver only emits events for changes to content area, not scroll area. The content width of the doc table wrapper doesn't change since it's scrollable. So implementing a solution with ResizeObserver would add significant complexity to the directive and I'm not confident it would work correctly in all cases. You'd have to know for a fact which element is going to overflow the container, apply the observer to that element, and somehow let the container know to update its scroll bar. But this would fail if any other child element happened to overflow, and it would also fail if the window changes size while the content remains the same.

So... I guess I'm back to thinking my previous implementation is the best we can do at the moment. I'd like to hear your feedback on my previous comment!

weltenwort · 2016-12-16T10:56:43Z

Too bad about the ResizeObserver.

After reading a bit more about it, I have to agree that display: table probably won't help us here. This sounds like the exact use case for flexbox with everything except for the discover table having a fixed height while the table being flex: 1 0 0. Looking at the DOM, the body > .content > .app-wrapper > .app-wrapper-panel > .application hierarchy already is made of flexboxes that fill the full height. Too bad that extending the flexbox model through the rest of the .app-container would have such negative performance impact.

Regarding the timeout solution: It is indeed probably not worse than before. Another option would be to _.debounce the watcher in the previous implementation.

Bargs · 2016-12-16T19:01:05Z

@weltenwort can you elaborate on what you mean by debouncing the watcher? Are you referring to the callback, or the functions that return the watched values?

The watched functions are the cause of the forced layouts since they check the width of the element. If I debounce those on the trailing edge they'll return out of date values. If I debounce them on the leading edge the forced layout will still occur during the digest cycle. Maybe I'm misunderstanding something though.

weltenwort · 2016-12-19T10:13:13Z

True, it might be problematic to debounce the functions that return the watched values. I was thinking of a compromise in which we perform the comparison inside the checkWidth function the way you implemented it in this PR so far, but debounce it and trigger it unconditionally during the digest cycle. That way the recalculation would still be bound to changes in the displayed content instead of constant polling. Not sure if the noop of the debounced function is still too costly to execute on every digest though.

Bargs · 2016-12-19T17:01:00Z

@weltenwort just pushed a commit using debounce. Is this what you were thinking of? Seems to work well, I'm good with that implementation if you are.

weltenwort

Yes, that's what I meant. Sorry for not being clear enough.

weltenwort · 2016-12-19T18:08:26Z

src/ui/public/directives/__tests__/fixed_scroll.js

@@ -9,10 +9,12 @@ import Promise from 'bluebird';
 describe('FixedScroll directive', function () {

  let compile;
+  let timeout;


Does this still work when using _.debounce instead of $timeout?

Our angular friendly version of debounce actually uses $timeout under the hood so I still need to flush those timeouts to get the tests to work.

I've removed the timeout flushing and mocked debounce instead so that the tests don't need to know anything about debounce's implementation.

TIL we have a custom debounce. Thanks!

weltenwort · 2016-12-19T18:08:33Z

src/ui/public/fixed_scroll.js

+        }
+
+        scrollWidth = newScrollWidth;
+        width = newWidth;


For slightly more performance optimization these reassignments could go into the if block. I'm not in favour of those state variables in general, but can't think of a better way. 😉

…rnals

Bargs · 2016-12-21T15:14:41Z

@weltenwort @lukasolson so, how are we looking?

weltenwort

LGTM, good improvement for such small code changes

lukasolson

LGTM!

Improves responsiveness by avoiding a couple of forced reflows. * Only renders JSON doc view when requested, instead of rendering it as soon as the row is expanded. * Improves fixedScroll directive by avoiding width/height checks during the digest cycle. (cherry picked from commit b6969f5)

Bargs added review v5.2.0 v6.0.0 labels Dec 1, 2016

Bargs assigned epixa and lukasolson Dec 1, 2016

lukasolson reviewed Dec 2, 2016

View reviewed changes

Bargs mentioned this pull request Dec 6, 2016

Truncate _source summary in doc table #9394

Closed

tbragin added the :Discovery label Dec 14, 2016

lukasolson assigned Bargs and unassigned epixa and lukasolson Dec 14, 2016

Bargs force-pushed the docViewerPerf branch from d3170f6 to dbca442 Compare December 15, 2016 17:11

Bargs force-pushed the docViewerPerf branch from dbca442 to 6c75e22 Compare December 19, 2016 16:56

Bargs added 4 commits December 19, 2016 11:58

Only render JSON view when the user clicks the tab

666c788

Decouple width polling from digest cycle

3509be1

documentation

cd58a4f

Flush those timeouts!

b642ba6

Use debounce instead of timeout

a260f69

Bargs force-pushed the docViewerPerf branch from 6c75e22 to a260f69 Compare December 19, 2016 16:59

weltenwort reviewed Dec 19, 2016

View reviewed changes

Bargs added 2 commits December 19, 2016 16:25

move assignment into if block

bb7369d

Mock debounce so the tests don't need to know anything about its inte…

78090bc

…rnals

weltenwort approved these changes Dec 21, 2016

View reviewed changes

Bargs assigned lukasolson and unassigned Bargs Dec 22, 2016

lukasolson approved these changes Dec 27, 2016

View reviewed changes

lukasolson assigned Bargs and unassigned lukasolson Dec 27, 2016

Bargs merged commit b6969f5 into elastic:master Dec 27, 2016

elastic-jasper mentioned this pull request Dec 27, 2016

[5.x] Reduce lag experienced when expanding doc table rows #9663

Merged

Bargs mentioned this pull request Dec 29, 2016

Fix error switching index patterns in Discover #9682

Merged

weltenwort mentioned this pull request Jan 24, 2017

Avoid infinite digest loop caused by $watch and $timeout #10036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce lag experienced when expanding doc table rows #9326

Reduce lag experienced when expanding doc table rows #9326

Bargs commented Dec 1, 2016

Bargs commented Dec 2, 2016

Bargs commented Dec 2, 2016

lukasolson Dec 2, 2016

lukasolson Dec 2, 2016

Bargs Dec 2, 2016

Bargs Dec 2, 2016

lukasolson commented Dec 8, 2016 •

edited

Loading

Bargs commented Dec 8, 2016

weltenwort commented Dec 9, 2016

Bargs commented Dec 10, 2016

weltenwort commented Dec 12, 2016

Bargs commented Dec 12, 2016

Bargs commented Dec 12, 2016

Bargs commented Dec 15, 2016

weltenwort commented Dec 16, 2016

Bargs commented Dec 16, 2016

weltenwort commented Dec 19, 2016

Bargs commented Dec 19, 2016

weltenwort left a comment

weltenwort Dec 19, 2016

Bargs Dec 19, 2016

Bargs Dec 19, 2016

weltenwort Dec 20, 2016

weltenwort Dec 19, 2016

Bargs Dec 19, 2016

Bargs commented Dec 21, 2016

weltenwort left a comment

lukasolson left a comment

Reduce lag experienced when expanding doc table rows #9326

Reduce lag experienced when expanding doc table rows #9326

Conversation

Bargs commented Dec 1, 2016

Bargs commented Dec 2, 2016

Bargs commented Dec 2, 2016

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

lukasolson commented Dec 8, 2016 • edited Loading

Bargs commented Dec 8, 2016

weltenwort commented Dec 9, 2016

Bargs commented Dec 10, 2016

weltenwort commented Dec 12, 2016

Bargs commented Dec 12, 2016

Bargs commented Dec 12, 2016

Bargs commented Dec 15, 2016

weltenwort commented Dec 16, 2016

Bargs commented Dec 16, 2016

weltenwort commented Dec 19, 2016

Bargs commented Dec 19, 2016

weltenwort left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bargs commented Dec 21, 2016

weltenwort left a comment

Choose a reason for hiding this comment

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson commented Dec 8, 2016 •

edited

Loading