-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance Optimization #366
Comments
@BorisMoore here's the jsfiddle https://jsfiddle.net/q74wxy8v/2/ Please let me know if there's anything else you need. Thank you! |
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:
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 |
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. |
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.
We replaced that with
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. |
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:
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. |
Oh thank you @Paul-Martin very nice ideas. I'll try those and will let you know if performance improves. |
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 This one seems expensive (25% of total time):
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. |
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 |
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? |
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? |
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... |
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. |
Sure, I'll close it. You can still report back here even if it is closed. Or we can re-open it as appropriate... |
I plan to provide for async deferred data-linking after V1. See #368. |
Created this one to replace misplaced issue BorisMoore/jsrender#312
Will update you when jsfiddle is ready.
Thank you again 😄
The text was updated successfully, but these errors were encountered: