BorisMoore / jsviews

Interactive data-driven views, MVVM and MVP, built on top of JsRender templates
http://www.jsviews.com/#jsviews
MIT License
856 stars 130 forks source link

Performance Optimization #366

Closed markibanez closed 7 years ago

markibanez commented 7 years ago

Created this one to replace misplaced issue https://github.com/BorisMoore/jsrender/issues/312

Will update you when jsfiddle is ready.

Thank you again :smile:

markibanez commented 7 years ago

@BorisMoore here's the jsfiddle https://jsfiddle.net/q74wxy8v/2/

Please let me know if there's anything else you need. Thank you!

Paul-Martin commented 7 years ago

For reference this jsfiddle takes 3.3 seconds on a 2011 Macbook pro using chrome, 5.1 using firefox, 2.4 using Safari. Not exactly state of the art hardware. Based on my experience with tables in my own product using jsviews, and looking at the level of data-linking in your template, I'd say thats pretty much right on the mark. If you are truly seeing 12 seconds then I would consider the following items:

  1. Do you happen to have processes using up available CPU on your machine when running these tests?
  2. What browser are you using? IE doesn't perform as well as other modern browsers and we see this in our user base as well - though not to the extent of a 4x difference. Try chrome.
  3. Are you on windows? If so, are you running anti virus? In our customer base we've seen some (most honestly) antivirus software have a negative impact on javascript performance at this level. Unfortunately modern AV packages like to insert plugins to browsers that can dramatically impact performance. I'd suggest temporarily disabling and see if that brings your results inline with mine. (If they do, consider windows defender - it doesn't have the extent of issues other AV has in terms of performance in my experience)
  4. Do you have a large number of browser extensions/plugins enabled? I find this to be especially common with developers. Sometimes one of those can be poorly written and cause terrible performance. I've run into to this in our customer base as well. Try disabling plugins to see if you get performance similar to mine.
  5. Try running the tests with browser debuggers turned off.

Unfortunately even if one of the above turns out to be true, you may not have the ability to influence browser, AV, plugins etc with the users that are going to use your product. So the above steps are really just intended to help you isolate what is causing the difference between what you are seeing and what I'm seeing.

Ultimately you are probably going to have to incrementally render or paginate (show first 50 or 100 rows, and add a 'show more') or significantly reduce the amount of data-linking that is happening per row. If you have the choice between true pagination or just a show more, I'd recommend doing the show more route -- e.g. append rows to the existing, not replace. This is because when replacing all those table rows you will find a new performance problem that crops up in jQuery itself in jQuery.cleanData. That function is very slow (google it to see the complaints) but when nodes are disposed it processes every node to remove the various data items it has appended. In my product, which has a similar amount of data-linking within a table, I found if rendering 100 rows took X, replacing and rendering a new set of 100 rows would take 1.5-2X. Tables have a way of producing way more links than you realize very quickly when you consider X rows Y cells Z links per cell.

Here is an updated jsfiddle that shows the performance you can expect if you render the first 100 items and then add logic to show more (append the next 100, etc). On my machine using chrome I get .6 seconds for initial render and .6 seconds for the next 100. (Note this doesn't have functional show more code - it just renders 100, then appends 100 and times each)

https://jsfiddle.net/q74wxy8v/3/

This fiddle shows what you can expect doing pagination (replace original 100 rows).

https://jsfiddle.net/q74wxy8v/4/

On my machine it is .6 and .9 seconds

markibanez commented 7 years ago

Hey @Paul-Martin thank you for that very exhaustive comment. Everything you said made absolute sense. I retested on my own computer and got faster results. I think our development server may be really really slow.

Just as I suspected, I don't think we can squeeze any more performance for the amount of data-linking in the code.

Anyway I have updated the jsfiddle here https://jsfiddle.net/q74wxy8v/5/ as it wasn't a good idea to use Google Drive as a CDN and did by the time I opened your forks. For @BorisMoore too as you might want to see the fiddle.

Paul-Martin commented 7 years ago

You're welcome.

One other technique I've found that can be helpful with tables -- and it doesn't appear to apply to your example -- is to move data-linked classes to rows wherever possible and use CSS parent / child / nth-child formatting. In cases where you may have lots of TDs this can be a huge win. For example we at one point where using 4 or 5 merged classes on individual TDs to affect border formatting, etc.

<td data-link="class{merge:foo toggle='bar'} class{merge:bar toggle='foo'} ...">{^{:something}}</td>

// css
td.foo {
  ... something..
}

td.bar {
  ... something else ...
}

td.bar:nth-child(2) {
 ... any something else ...
}

We replaced that with

<tr data-link="class{merge:foo toggle='bar'} class{merge:bar toggle='foo'} ...">
  <td>{^{:something}}</td>
</tr>

// css
tr.foo td {
  .. something ..
}

tr.bar td {
  .. something else ..
}

tr.bar td:nth-child(2) {
 ... any something else ...
}

This was a huge performance win as we reduced the number of links by 100 rows 30 cells 5 links per cell = 15000, versus 100.

So to the extent that you might be able to move formatting related css classes from TDs to TRs you can get some really big wins.

Paul-Martin commented 7 years ago

Sorry - one more trick - looking through git logs to jog my memory.

If you allow columns to be hidden or shown, and you have a lot of data-links on each cell, you can use an {^{if}} statement to conditionally renders an empty, hidden TD, versus a normal TD with all the data linking. If for example you have 5 links per TD and 100 rows, you can save 500 links for each non-visible column:

{^{if !show}}
  <td class="hidden"></td>
{{else}}
  <td data-link="class{merge:foo toggle='foo'} .. etc ..}>{^{:something}}</td>
{{/if>

This technique also ended up being a big win. But notice you are trading adding an addition link via the {^{if}} statement for every TD, so you have to look at typical use patterns to see if that is a net win for you. In our product we have up to 30 columns, but by default only 3-7 are displayed, so it was a big net win for the nominal case.

markibanez commented 7 years ago

Oh thank you @Paul-Martin very nice ideas. I'll try those and will let you know if performance improves.

BorisMoore commented 7 years ago

Great discussion. Thanks!

Yes it looks like there is a lot of work going on rendering and linking content that is initially hidden. Any place where you are using class toggling to hide/show (toggle="hidden"} may be better implemented as {^{if ...}}...{{/if}} or data-link="{if ... tmpl=...}".

This one seems expensive (25% of total time):

<td class="{{:~cs.Numerics.indexOf(prop.tid) > -1 ? 'cell-align-right' : ''}}" data-link="class{merge:!prop.show || prop.tid == 18 toggle='hidden'}">

and is inside a tr whose content is I think initially hidden - so it would be helped by changing the tr to use conditional if for hiding.

Any reason you can't do the initial linking of just 50 rows or so (the max that might show in a browser) and then use observable insert for the all the remaining rows, but wrapped in a setTimeout, or some kind of async wrapper or promise? Run other code on just the 50 rows, or after the promise resolves, as appropriate.

markibanez commented 7 years ago

Hey thanks. In the "actual" code we do exactly that, I just show the empty table first, then show a loading screen until everything completes. It looks like I can move several classes to tr from td. I can't believe I didn't think of that.

Appreciate both your time @BorisMoore and @Paul-Martin

BorisMoore commented 7 years ago

Right, but if you want, can you load just 50 or 100 rows, then remove the loading screen, and let the rest of the rows load async? It would just mean the scroll bar would show the second batch of content arrive, and until then, they would not be able to scroll all the way to the bottom... Do you need to hide the table until all 600 rows are available?

markibanez commented 7 years ago

I've tried loading partially and adding rows when they scroll to the end. I think it's the same concept. But I found it was too confusing especially when we get to sorting and filtering. Any ideas about incorporating sorting and filtering to your idea? Possibly disable sorting and filtering when everything's not loaded yet?

BorisMoore commented 7 years ago

I wasn't meaning to wait until they scroll. Have it all load in the background, but provide access to the first screenful, while the rest is loading. Disable the sort and filter options until all rows are available...

But it depends on how long it takes. Initial time 1 second then full table available after 5 seconds more is OK, probably, but if it is 20 seconds wait that may get confusing. (Enough time for them to try and fail to access the filtering...).

For example on jsviews.com I do background loading of other 'pages' (reached through top level menus) and of search indexes, etc. By the time people are ready to switch top-level menus or do a search, it will all be loaded anyway. But the perceived perf is much better.

So you will need to evaluate alternative user experiences of the different possible approaches... Maybe your wait screen is best...?

Sometimes one can do server rendering of initial content, (all the rows of your table for example) but it is not 'live' until the data has loaded in the background. So clicking filter would initially either be disabled, or will have a slow response the first time, as it waits for the data to be 'ready'. But of course in your case you are probably not set up to switch to that architecture...

markibanez commented 7 years ago

All very good points @BorisMoore which I now am able to think about thanks to you. I will test all of your comments and let you know how it goes. In the meantime, I'm not sure if we should close this issue or not.

BorisMoore commented 7 years ago

Sure, I'll close it. You can still report back here even if it is closed. Or we can re-open it as appropriate...

BorisMoore commented 7 years ago

I plan to provide for async deferred data-linking after V1. See #368.