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

Bad performance when selecting a huge range #778

Closed
rojepp opened this issue Oct 9, 2013 · 11 comments
Closed

Bad performance when selecting a huge range #778

rojepp opened this issue Oct 9, 2013 · 11 comments

Comments

@rojepp
Copy link

rojepp commented Oct 9, 2013

When loading 8 million rows, selecting a range of about half of them will crash the browser tab in Chrome, Firefox, probably all of them.

Try it out in this reduced sample: http://jsfiddle.net/D9TYR/1/

  • Select one of the first rows
  • Scroll about halfway down
  • Shift-click to select a range.
  • Tab hangs

The real code uses RemoteModel and also exhibits the problem.

Can the performance here be improved?

@mleibman
Copy link
Owner

It can, but considering the borderline unrealistic expectations, I can't really prioritize this over other work.

However, if you really insist on implementing a web app that tackles 8M data items, you are likely to hit a thousand other bottlenecks in a bunch of areas :) In case you do decide to tackle it, I'll provide you with the rough plan of attack below.

The current bottlenecks in SlickGrid are:

  1. Selection models not doing range merging (i.e. selecting 5 rows will create 5 selection ranges instead of one covering all 5 rows).
  2. Grid converting ranges to rows (for developer convenience). This also creates an array of individual selected rows. This calculation can be made on demand instead.
  3. Grid using setCellCssStyles() to mark selected cells. This requires pre-computing the hash for the entire selection, but can be implemented using the getItemMetadataProvider() so that the determination is done on the fly.
  4. The grid needs to call canCellBeSelected() for each cell (not row!) to check if it can, in fact, be selected. For older browsers that don't have advanced JS compilers with function inlining, this is going to be the killer.

@rojepp
Copy link
Author

rojepp commented Oct 12, 2013

Thanks for your thorough answer!
I realize that these expectations might not be common, but it seems within reach to comfortably handle any number of rows. :)

  1. Selection models not doing range merging (i.e. selecting 5 rows will create 5 selection ranges instead of one covering all 5 rows).

I noticed this too. I was wondering why ranges are used at all? I haven't looked at all of the code, so forgive me if I'm missing something. Why not just an array of selected indexes?

  1. The grid needs to call canCellBeSelected() for each cell (not row!) to check if it can, in fact, be selected. For older browsers that don't have advanced JS compilers with function inlining, this is going to be the killer.

This is what kills it, even on modern browsers. Latest Chrome and Firefox chokes here. This could be done on the fly, when displaying rows, given an array of selected indexes, right?

@mleibman
Copy link
Owner

  1. The API is designed for flexibility and scalability, but the actual implementation takes a shortcut (which was perfectly reasonable since, in the last ~4 years of SlickGrid's existence, I've only gotten one complaint (: ).

  2. In my benchmark, latest Chrome and FF actually done just fine there due to inlining. The real bottleneck right now for these browsers are points 1 - 3. It's only when you eliminate them that you'll hit upon 4. Anyway, I don't think this can be done on the fly though, since you shouldn't be able to select the unselectable cells. It's not just a matter of rendering/styling. There are still ways around this of course, either by expanding the API to make this optional or declarative (i.e. eliminate the function call), or by employing tricks like JIT compilation similar to what DataView does for filters. Of course, within a couple of years, the problem will fix itself since all typically supported browsers will have sufficiently advanced JS compilers :)

@rojepp
Copy link
Author

rojepp commented Oct 12, 2013

  1. canCellBeSelected() can also be called on the fly when actually displaying rows? I'm crashing here, not the other points. I think it may have to do with the amount of memory being used when looping over rows and cells..

@mleibman
Copy link
Owner

Selection isn't just a matter of updating the UI. It's part of the API. It needs to tell the truth, so a selection model CANNOT set the selection to something that is unselectable.

@mleibman
Copy link
Owner

Also, "crashing" is not a good indicator of where things go wrong. Reduce the size to 500K and benchmark both performance and memory use.

@rojepp
Copy link
Author

rojepp commented Oct 12, 2013

Selection isn't just a matter of updating the UI. It's part of the API. It needs to tell the truth, so a selection model CANNOT set the selection to something that is unselectable.

But it already does? Rows are selected, only after that are individual cells interrogated for their ability to be selected. I'm suggesting that this interrogation could be done lazily instead of greedy.

I noticed that if I limit the cell checking (and setting style) to the first column, that it at least worked. It wasn't fast, but it wasn't terrible either.

@mleibman
Copy link
Owner

No, it does NOT do it already. As I already mentioned, it has to do it
right away in order to translate user action to what it interprets as
"selection", so that it can fire the 'onSelectedRangesChanged' event with
it. It cannot be done lazily.

On Oct 12, 2013 1:25 PM, "Robert Jeppesen" notifications@github.com wrote:

Selection isn't just a matter of updating the UI. It's part of the API.
It needs to tell the truth, so a selection model CANNOT set the selection
to something that is unselectable.

But it already does? Rows are selected, only after that are individual
cells interrogated for their ability to be selected. I'm suggesting that
this interrogation could be done lazily instead of greedy.

I noticed that if I limit the cell checking (and setting style) to the
first column, that it at least worked. It wasn't fast, but it wasn't
terrible either.


Reply to this email directly or view it on GitHubhttps://github.com//issues/778#issuecomment-26205287
.

@oghanem
Copy link

oghanem commented Oct 14, 2013

"However, if you really insist on implementing a web app that tackles 8M data items, you are likely to hit a thousand other bottlenecks in a bunch of areas :)"

I tried this with < 1 000 000 rows and it crasched!

@mleibman
Copy link
Owner

In profiling the select-all for 500K rows with the row selection model, canCellBeSelected() calls only contribute 6.24% of the time spent. On my MacBook Air in Chrome, selecting everything takes 1.4 seconds. FF was only half a second slower. Not too terrible.

At this point, I'm going to have to say that I'm not going to pursue this any further. Practically nobody needs the support for these numbers of items, and I'm perfectly fine with good selection performance of at least 100K. That said, I believe I provided you with a plan on how you can improve the selection provider (or write a new one geared towards large selections) and the grid to support a practically unlimited selection if your app requires it. If you do end up making those changes, I'm always open to contributions :)

@wvary
Copy link

wvary commented Sep 20, 2014

I know I'm late to the game and I'm only loading 250K of rows.
The select all checkbox takes about 1.5 seconds to execute the rowstorange function.
Then there's another small delay in highlighting the displayed rows.
I don't have pagination on, I do have scrollable with max height.
Currently, I'm bench testing the library so I only have two columns shown, the checkbox and one data column.
I have a mac book with i7 and 16gb ram running chrome.
The is consistent with what mleibman found.
The slight delay when toggling check all (to on only, off is instantaneous) is not acceptable in my application.
I'll if there are improvements to be made. If so, I'll submit it for reviews.
I need to find a Javascript grid/table library that can handle large data (100K rows or more) and so far, SlickGrid appears to be the candidate with a few issues to overcome.

Thanks

jesenko pushed a commit to plandela/SlickGrid that referenced this issue Dec 29, 2023
- rollback PR mleibman#769 that brough event passive mode, it turns out that all our SlickGrid events can use `preventDefault` which is what passive mode is for, so it's better to rollback all of it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants