adopted-ember-addons / ember-light-table

Lightweight, contextual component based table for Ember
http://adopted-ember-addons.github.io/ember-light-table/
MIT License
312 stars 131 forks source link

Performance problems #12

Open SirZach opened 8 years ago

SirZach commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago
  {{#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 commented 8 years ago

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 commented 8 years ago

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

SirZach commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago
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.

taras commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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

API:

Ember:

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 commented 8 years ago

@cball this is great. Thank you!

runspired commented 8 years ago

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 commented 8 years ago

@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.

nmcclay commented 8 years ago

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 commented 8 years ago

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

vasilakisfil commented 8 years ago

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

xcambar commented 7 years ago

@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 commented 7 years ago

@vasilakisfil @offirgolan in our project relying on ember-light-table@1.4.0 @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 commented 7 years ago

Interesting ! Could it be new glimmer rendering engine ? On Oct 6, 2016 18:49, "Eli Flanagan" notifications@github.com wrote:

@vasilakisfil https://github.com/vasilakisfil @offirgolan https://github.com/offirgolan in our project relying on ember-light-table@1.4.0 @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 https://github.com/offirgolan/ember-light-table/issues/209 for fix

needed before trying)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/offirgolan/ember-light-table/issues/12#issuecomment-252021928, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkC79iPey_IxsPY5FNjzYcK_NxFfs6iks5qxSaYgaJpZM4IMA7x .

offirgolan commented 7 years ago

@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 commented 7 years ago

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

runspired commented 7 years ago

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

runspired commented 7 years ago

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

offirgolan commented 7 years ago

@runspired I would love to chat more about this when we both have some free time 😄

ghost commented 7 years ago

@offirgolan thank you for merging those fixes so quickly; no worries on not having tested beta. That is what having the project open source is for 😄 @vasilakisfil most certainly.

ghost commented 7 years ago

for more clarity talking about performance here, I created a simple ember app with a table component, mock data list, and a test case that measures speeds using window.performance: https://github.com/efx/poc-bug/. To see the average render time, run ember test and open up a a browser console. In my runs, the average render is around 2.5 to 2.6 seconds. (Observed the 100 millisecond boost when disabling prototype extensions. )

runspired commented 7 years ago

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

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

andreikun commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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:

runspired commented 7 years ago

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

andreikun commented 7 years ago

@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 commented 7 years ago

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

andreikun commented 7 years ago

@offirgolan that sounds great! Thanks!

patrickberkeley commented 7 years ago

FWIW as a workaround, we paginated our data.

bugduino commented 7 years ago

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

billdami commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

runspired commented 7 years ago

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

dougdellolio commented 7 years ago

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

markcellus commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

Sorry for the wait -- https://github.com/offirgolan/ember-light-table/pull/483 Making progress :railway_track: :railway_track: :steam_locomotive: :railway_track: :railway_track: :railway_track: