Skip to content
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 problems #12

Open
SirZach opened this issue Apr 20, 2016 · 50 comments
Open

Performance problems #12

SirZach opened this issue Apr 20, 2016 · 50 comments

Comments

@SirZach
Copy link

SirZach commented Apr 20, 2016

Hi again,

I've noticed a performance problem and I haven't figured out what is causing it yet. I tried attaching the profile but github doesn't allow .cpuprofiles. You can easily reproduce it by profiling the demo table at http://offirgolan.github.io/ember-light-table/#/. The rendering speed isn't that noticeable since you're only rendering ~20 rows off the bat but even at ~150 rows, rendering speed is at ~2.5 seconds. I've reduced my table down to one row and have noticed in the profile that the one row creates about 3 large spikes that have a huge number of function calls. You can see spike after spike, for each row, if you profile the demo page.

I'm continuing to investigate. We really like ember-light-table but this performance issue is a show-stopper at the moment. Cheers,

Zach

@offirgolan
Copy link
Collaborator

Hi Zach. In terms of performance, one of the biggest issues is not being able to render a non existent component. If you look at the code base, we are still forced to create and render each invisible component. This means that for each row, we actually render 3 (although 2 of which are just comments, we are still forced to create two additional views). I've spoken to @rwjblue about this and it seems to be a limitation with the component helper not being able to accept null as a component name.

In terms of profiling, can you provide a snippet of your table setup so that way we are all profiling the same thing? I'm currently on vacation but I'll be able to look at this early next week.

@SirZach
Copy link
Author

SirZach commented Apr 20, 2016

  {{#netuitive-table-new model=model columns=columns as |ntn|}}
    {{#light-table ntn.table tableActions=(hash checkAll=(action 'checkAll')) as |t|}}
      {{t.head
        onColumnClick=(action ntn.onColumnClick)
        iconAscending='fa fa-sort-asc'
        iconDescending='fa fa-sort-desc'
      }}

      {{t.body
        canSelect=false}}

    {{/light-table}}
  {{/netuitive-table-new}}

netuitive-table-new simply takes care of initing the Table and making it available. For consistent profiling though, profiling the table on your demo site, http://offirgolan.github.io/ember-light-table/#/ shows exactly what I'm seeing locally. Thanks for replying even while on vacation!

@SirZach
Copy link
Author

SirZach commented Apr 20, 2016

Do you know off the top of your head where in the codebase it creates and renders each invisible component? I've poked through it quite a bit and I'm not sure I've seen that or know exactly what you mean.

@offirgolan
Copy link
Collaborator

See addon/templates/components/lt-body.hbs

@SirZach
Copy link
Author

SirZach commented Apr 20, 2016

Ok, I actually extended that component and the light-table component to reference my extended one which removed all references to yield for kicks and giggles and didn't notice any performance difference.

EDIT: I made a lt-cell-light component to use instead of lt-cell that doesn't implement the init function and rendering speed when down from about 2.2 seconds to 1.2 seconds.

@offirgolan
Copy link
Collaborator

@SirZach makes sense... defineProperty is pretty slow. That would be a good place to start optimizing but I dont really know a better replacement. Ill do some investigation and see what I can do.

cc @stefanpenner

@offirgolan
Copy link
Collaborator

This has been addressed and to my knowledges resolved with #14. Gonna go ahead and close this for now. Feel free to reopen if you face any more performance issues.

@SirZach
Copy link
Author

SirZach commented Jun 6, 2016

screen shot 2016-06-06 at 12 26 29 pm

Still seeing the very long columns when profiling your demo page. The table seems fast despite the columns but I think you really start feeling the pain when you have hundreds or thousands of items.

@offirgolan offirgolan reopened this Jun 6, 2016
@offirgolan offirgolan added this to the 1.0.0 milestone Jun 13, 2016
@taras
Copy link
Collaborator

taras commented Jun 13, 2016

Would limiting how much is rendered address this problem? For example, we could use something like smoke-and-mirrors to render only the rows that the user can see. Would this address the issue with having thousands of rows?

@SirZach
Copy link
Author

SirZach commented Jun 13, 2016

@taras I think something like smoke-and-mirrors would be great although I do believe that the project is planning on dropping support for tables for the 1.0 release. Regardless, the columns in the attached image are simply too much for rendering even small amounts of data, about 25-50 rows.

@taras
Copy link
Collaborator

taras commented Jun 13, 2016

@cball mentioned API considerations that he had to make for ember-spreadsheets to make rows and columns performant.

@cball, would you mind sharing your findings that might be relevant to this?

@cball
Copy link

cball commented Jun 14, 2016

A few things I've learned related to performance with big tables:

API:

  • try to return the table data as needed to display. The less transformation you have to do client-side the better.
  • use plain arrays rather than relationships

Ember:

  • with many hundreds or thousands of records don't use Ember Data
  • the more template driven things are, the slower renders will be. I've tried a fully template driven approach for generating column definitions and it didn't work out that well. Maybe Glimmer 2 will make this a non-issue. I think this addon has a good balance of template driven and class driven approaches though so it might not be impacted as much.
  • smoke and mirrors is great, but calculations can get out of whack when measuring table children. If this happens, the side-effect seems to be random whitespace. This is probably why as @SirZach mentioned support for tables might be dropped in the future. It also only works in 1 direction.
  • I've tried a spike using a smoke-and-mirrors-like approach that works in both directions, using getClientRect within requestAnimationFrame to avoid adding scroll listeners. This works pretty well. The general approach here would be to check each header to determine if it's still in the viewport. This lets you figure out which columns should render. Rows are a similar concept.
  • unbind whatever you are able to using unbound

Ideally the Ember things would be used only when running into issues, I don't think that they should be a default. Especially considering most apps don't need to support tables of this size. Maybe we add {{ember-enterprise-table}}. :trollface:

Generally speaking, I'd try and see if there is a different UI design that allows for a more rolled up view of the data, but sometimes you have no choice.

Not sure if any of this helps, but there's a braindump for you. 😄

@taras
Copy link
Collaborator

taras commented Jun 14, 2016

@cball this is great. Thank you!

@runspired
Copy link
Contributor

Table support will be in smoke-and-mirrors 1.0, but not via the main component, it does have it's own edge cases.

I've tried a spike using a smoke-and-mirrors-like approach that works in both directions, using getClientRect within requestAnimationFrame to avoid adding scroll listeners. This works pretty well. The general approach here would be to check each header to determine if it's still in the viewport. This lets you figure out which columns should render. Rows are a similar concept.

If you look into the virtual-item branch of smoke-and-mirrors, the radar is extractable (and I've started to do so at ember-radar https://github.com/runspired/ember-radar). This is essentially an IntersectionObserver polyfill. If @cball want's to champion helping extract and finalize radar as an addon, very willing to turn over the reigns.

unbind whatever you are able to using unbound

unbound and inline if continue to be two of the best in-template perf boosters there are.

try to return the table data as needed to display. The less transformation you have to do client-side the better.
use plain arrays rather than relationships

These are ED issues more than anything else, I have a p.o.c. that shows for large quantities of data we could be roughly 1000x faster with much lower memory overhead than ED is (includes having relationships, and improves the sparse relationship story). These are changes I'm plotting to help bring to ED itself. I'd be willing to clean up the p.o.c. and release it as something like "ember-light-store" or some such.

I would add to this that I've consistently found the biggest bottleneck in long lists / large data sets is not render, but the instantiation cost of Ember.Object and subclasses of it. Turning off prototype extensions and converting your data structures to well shaped, simpler classes if you can results in a very significant speed up.

@offirgolan
Copy link
Collaborator

@runspired that sounds super exciting! Once S&M hits 1.0 we can think of restructuring the internals of this table to support occlusion rendering.

One more thing that I want to note, which I believe can be a pretty big cause of the rendering performance problems is not being able to render "null" components. Looking at the tbody of this addon:

{{#each rows as |row|}}
  {{lt-row row}}

  {{yield (hash
    expanded-row=(component 'lt-spanned-row' classes='lt-expanded-row' colspan=colspan yield=row visible=row.expanded)
    loader=(component 'lt-spanned-row' visible=false)
    no-data=(component 'lt-spanned-row' visible=false)
  ) rows}}
{{/each}}

{{yield (hash
  loader=(component 'lt-spanned-row' classes='lt-is-loading' colspan=colspan)
  no-data=(component 'lt-spanned-row' classes='lt-no-data' colspan=colspan)
  expanded-row=(component 'lt-spanned-row' visible=false)
) rows}}

Everytime we yield, we must yield all 3 body contextual components which use visible=false to not render anything at all. Although this is what we need visually, the invisible components still get instantiated which shows up as this in the DOM:

screen shot 2016-06-15 at 10 44 45 am

As you can see from the above screenshot, For every single rendered row, there are actually 3 components being instantiated and only one being rendered.

Currently, there is no way to use the component helper with a component that doesnt actually exist. I've talked about this with @rwjblue in hopes to support something like yield (component 'does-not-exist') in the near future.

@offirgolan offirgolan modified the milestone: 1.0.0 Jul 6, 2016
@nmcclay
Copy link
Contributor

nmcclay commented Aug 15, 2016

I also encountered some of the performance limitations discussed here. One temporary solution I found that yielded significant results was dropping out ember-scrollable and ember-wormhole from the lt-body.hbs lt-head.hbs & lt-foot.hbs. Obviously there are some additional style tweaks and some loss in functionality of ember-scrollable, but the performance benefits are about 2X faster from my personal testing (12s -> 7s on my smoke test table - 100 rows, lots of custom cell components). There is also clearly a performance relationship to the total number for columns, cell components & total rows but this seems a promising option if you are really hurting for performance.

@vasilakisfil
Copy link

vasilakisfil commented Sep 15, 2016

Thanks @nmcclay that improved my performance around 25%. Any other tip ? (I didn't find any lt-cell)

@vasilakisfil
Copy link

@offirgolan could glimmer 2 improve performance? :D have you tested anything ?

@xcambar
Copy link

xcambar commented Oct 6, 2016

@offirgolan FYI, I maintain a helper that allow to check whether a component name resolves to an actual component: https://github.com/xcambar/ember-cli-is-component

It allows :

{{#if (is-component componentName)}}
  do what you have to do
{{/if}}

It could help you working around the component helper only acccepting valid components names.

@ghost
Copy link

ghost commented Oct 6, 2016

@vasilakisfil @offirgolan in our project relying on [email protected] @travis-jm found substantial rendering increases by updating to ember 2.9.0-beta.4. In some cases a load time of 20 seconds down to 5 when rendering 500 rows. (see #209 for fix needed before trying)

@vasilakisfil
Copy link

vasilakisfil commented Oct 6, 2016

Interesting ! Could it be new glimmer rendering engine ?
On Oct 6, 2016 18:49, "Eli Flanagan" [email protected] wrote:

@vasilakisfil https://github.com/vasilakisfil @offirgolan
https://github.com/offirgolan in our project relying on
[email protected] @travis-jm https://github.com/travis-jm found
substantial rendering increases by updating to ember 2.9.0-beta.4. In some
cases a load time of 20 seconds down to 5 when rendering 500 rows. (see
#209 #209 for fix
needed before trying)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAkC79iPey_IxsPY5FNjzYcK_NxFfs6iks5qxSaYgaJpZM4IMA7x
.

@offirgolan
Copy link
Collaborator

@efx thats really great news! I've been so busy, I havent had time to test the latest beta. I'll try to help as much as I can to resolve the issue so we can be 2.9.0 ready 😄

@offirgolan
Copy link
Collaborator

@efx v1.4.2 has been released with the updated dependencies.

@runspired
Copy link
Contributor

Even with glimmer2 you will now get a perf boost movin this to smoke and mirrors

@runspired
Copy link
Contributor

(I'd offer to help you work it in but I'm also busy on way too many things right now)

@runspired
Copy link
Contributor

ftr ember-wormhole currently does not work at all with glimmer2.

Also ftr, I'm close to having a fix in for it.

@andreikun
Copy link

andreikun commented Nov 14, 2016

I was able to improve performance up to 50% by removing lt-scrollable and ember-scrollable from lt-body file. Plus ember-wormhole but that does not seems to have any impact. Also i updated my project to ember 2.9

See attachments here:

  1. with lt-scrollable on lt-body.
    12
  2. without lt-scrollable on lt-body.
    13

This solution is to acceptable in case as i do not want to change stuff in library. Is there any other way to remove ember scrollable from this ? And also lt-scrollable.

@markcellus
Copy link

Very good stuff @andreikun! I'm seeing huge performance rendering issues when using a large set of rows with a lot of columns. How many rows and columns were you using in your testing?

I would assume that some sort of scroll handling needs to exist if rows overflow from the table's container. Although, not sure if ember-scrollable is necessary for that.

@andreikun
Copy link

Used 7 columns with 18 rows, also with my screen minimized to tablet viewport (900px width) to be able to add that row-toggle component (as in official responsive table example) to be able to display hidden columns for each row.

Also, it seems that it takes lot of time to render first column which is a column only for toggle

{
        width: '40px',
        sortable: false,
        cellComponent: 'row-toggle',
        breakpoints: ['mobile', 'tablet', 'desktop']
},

My last column also is rendering a component with 3 buttons inside.

{
    label: 'Actions',
    width: '110px',
    sortable: false,
    cellComponent: 'transaction-table-actions',
    breakpoints: ['desktop', 'jumbo']
}

See here performances.

14

@runspired
Copy link
Contributor

I'm almost ready for people to start trying out the latest smoke-and-mirrors, which has 5 major perf improvements in it and would address this:

  • it chunks the initial render (renders 1 new item per frame until total is met, helps ttfi and ttfp be faster)
  • it eliminated a number of scenarios which were causing extra backtracking-renders
  • it eliminates the need to keep a wrapper element behind for each item in the list (much lower DOM overhead)
  • it utilizes recycling for the items it does render
  • it no longer requires a wrapper element at all, you have full control over the "item"

@runspired
Copy link
Contributor

Also, Ember 2.10 beta with Glimmer2 will be substantially faster than Ember 2.9 @andreikun

@andreikun
Copy link

@runspired going to be patient and wait then for Ember 2.10 release. Really do not want to have performance/loading issues even on a 20 rows table, without even doing a request to server.

@runspired can you see any benefit of integrating smoke-and-mirros if i do not have an infinite scroll on my table ? Any idea how i can integrate this into light-table ?

Regards.

@offirgolan
Copy link
Collaborator

@andreikun that plan is to integrate smoke and mirrors into ELT once @runspired releases a stable version.

@andreikun
Copy link

@offirgolan that sounds great! Thanks!

@patrickberkeley
Copy link

FWIW as a workaround, we paginated our data.

@bugduino
Copy link

Any news on this? we still have some performance issues with a lot of rows using ember 2.11.0

@billdami
Copy link

Just chiming in here, as I'd love to an occlusion rendering solution baked into ember-light-table. :-)

@offirgolan it seems that the smoke-and-mirrors is being somewhat "abandoned" in favor of breaking it into several smaller components (https://github.com/html-next/vertical-collection), has this affected these plans at all?

Source: https://build.addepar.com/engineering/rendering-10-million-items-with-vertical-collection/

@offirgolan
Copy link
Collaborator

@billdami I've been looking into vertical-collection with @alexander-alvarez but we determined that it was still not fully stable for us to incorporate in this addon. I'll be experimenting with it in the next couple of weeks and hopefully come up with something.

@runspired
Copy link
Contributor

It should be at least as stable as smoke-and-mirrors at this point :P

@runspired
Copy link
Contributor

also, smoke-and-mirrors is absolutely not being abandoned :P

@dougdellolio
Copy link

@offirgolan any update on this? currently using in my app that has 1000+ rows and sadly isn't working 😢

@markcellus
Copy link

FWIW, there is a package based on S&M called vertical collection that may be used in regards to the suggestion someone left earlier about only rendering tables that are in the viewport as a user scrolls.

@alexander-alvarez
Copy link
Collaborator

Hey all... sorry for the radio silence.
we've been working on this, but progress has been slow due to inability to commit cycles to it. Both @offirgolan and I have been pretty slammed. I'm going to try to batch a large amount of work towards it this weekend, so if someone wants to catch up and help out, reaching out after then might be useful.

@buschtoens
Copy link
Collaborator

@alexander-alvarez I can't promise, that I'll be able to invest much time in the next week, but I'm very interested in getting this to work. It might be sensible to keep #284 in mind while implementing occlusion rendering depending on whether or not (and how) ember-scrollable can still be used.

@alexander-alvarez
Copy link
Collaborator

alexander-alvarez commented Jul 31, 2017

Hey all... sorry once again for the lack of communication.

I finally got around to dedicating an entire evening to tackling this problem from when I last had a couple of months ago (unfortunately, I had to refresh from where I left off). Tonight, I realized that the way I was approaching the problem -- drop in replacement & everything working -- meant we wouldn't see any progress on this piece of functionality, and would slow down it's delivery. So I've decided to break it up into more manageable pieces so that we can get the table out into people's hands as early as possible and then add functionality iteratively.

I'm building out a svelte-table in https://github.com/alexander-alvarez/ember-light-table/tree/vertical-collection that will have the same API as light-table (and will probably end up being exposed through a boolean flag on light-table), and will be built up of components that extend lt-head, lt-body etc and keep the APIs intact as much as possible.

That being said, the goal for my first PR to the repo will be a table that can uses vertical-collection in the body, and works when all columns specify a width. Other functionality will likely still be intact, but not guaranteed to be. I think by doing this as the first milestone we can all see progress on this piece of functionality that people can opt into a more perfomant, most likely less feature-rich component, and we can build it back up to the functionality on par with it's non performant counterpart.

Please stay tuned, and keep me honest so we can push this forward.

@xcambar
Copy link

xcambar commented Jul 31, 2017

that people can opt into a more perfomant, most likely less feature-rich component

Yes, that! Most (99%?) cases require performance OR features, not both (hence, "performance XOR features"). Someone may have a counterexample, but the approach you've described and started to work on, @alexander-alvarez, seems perfectly reasonable to me.

Congrats, I can't wait!

@alexander-alvarez
Copy link
Collaborator

Sorry for the wait -- #483
Making progress 🛤️ 🛤️ 🚂 🛤️ 🛤️ 🛤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests