Skip to content

Function to build on the fly #61

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

Closed
OscarGodson opened this issue Feb 2, 2016 · 9 comments
Closed

Function to build on the fly #61

OscarGodson opened this issue Feb 2, 2016 · 9 comments

Comments

@OscarGodson
Copy link

Clusterize handles scrolling very well, but a huge issue for us is initial load because for big datasets we have to build a string with tens of thousands of rows which locks up browsers and sometimes crashes it. Looks something like

var buildRows = function () {
    var tableData = [];

    rows.forEach(function (row, rowIndex) {
      var rowHtml = '<tr data-id="' + row.id + '">';

      row.data.forEach(function (cell, cellIndex) {
        rowHtml += '<td data-position="' + cellIndex + '">' + cell + '</td>';
      });

      rowHtml += '</tr>'
      tableData.push(rowHtml);
    });

    return tableData;
}

We also tried doing the above in "threads" with setTimeouts but what happens is each time we injected it into Clusterize it locked up because it had to build the rows and Clusterize had to re-render.

Is there a hook somehow to have Clusterize do the above on the fly when you build the DOM or if not, do you have any other suggestions on this?

@NeXTs
Copy link
Owner

NeXTs commented Feb 3, 2016

Hi Oscar

Please provide any simplified demo at codepen, this may help a lot for investigation.
As quick suggestion workaround.. how about:

  1. Init clusterize
  2. Build and insert rows to fill first cluster, let's say 100 rows so user will be able to see something in the list
  3. Maybe display some loading indicator at the bottom of the list (I'd made ​​it via pseudo-element)
  4. Meanwhile in background build all remaining rows in 'threads' with setTimeout, but not inject
  5. When all rows are build - inject it to the list at once

I'll think about better solution.

@NeXTs
Copy link
Owner

NeXTs commented Mar 9, 2016

@OscarGodson hi, how it's going?

@OscarGodson
Copy link
Author

We ended up use Web Workers which helped but is a bit more complicated. The solution of loading the first chunk then others would have required a lot of works on other things to know if it's in a loading state or not (like our search, sorting, etc). Web Workers at least sped it up by passing off the string creation to a new thread

@NeXTs
Copy link
Owner

NeXTs commented Mar 9, 2016

@OscarGodson
Try this version http://codepen.io/NeXTs/pen/vGGvRb
Also set option verify_change: true

@NeXTs
Copy link
Owner

NeXTs commented Mar 10, 2016

@OscarGodson hey, I'm waiting for your feedback :)

@OscarGodson
Copy link
Author

thanks, ill try to readd it and see how it works. Might be a little hard since it was all refactored to work with workers (sends the data async with events, etc) but I'll give it a shot

@OscarGodson
Copy link
Author

It appears to work! I think this + the workers would actually be a great hybrid because it'll speed up the workers even more. I need to do more speed tests but the initial feel seems like it renders rows faster than before.

@NeXTs
Copy link
Owner

NeXTs commented Mar 10, 2016

Great!
If you haven't changed clusterize sources - migration must be easy. It's non breaking improvement.

The main idea is to avoid unnecessary rerendering.
verify_change option was designed for cases like yours (do not re-render DOM if there were no updates in current cluster (rows that are currently in DOM) during .update()) but it didn't work properly.

I'll try to explain
Let's say we init our list with 1000 rows with default options. One cluster will fit approx 100 rows (so only 100 rows will be in DOM at one time)
For test purposes we do not scroll down and stay at the top of the list.
Then we append 50 rows more to the end of the list. Those 50 new rows will not be included in the current cluster anyway (in DOM) because current cluster may fit 100 rows only (100 first rows because we did not scroll).
It means that there no sense to re-render whole cluster (read whole list layout), because bottom offset changes only.

Previous comparison engine (comparison of old and new content to decide if need to re-render) did not consider those bottom offset (it was dummy checking of all resulting .outerHTML of the list (including tag with bottom offset)). So if bottom offset was changed to at least one pixel, checker noticed difference and decided to re-render full list.
New comparison engine checks for changes in content only and will rerender cluster if there were changes in at least one of including rows.
If bottom offset changes only - it will not rerender full list anymore, it will change single height property of single tag.
That's it.

I need some time to test all possible cases and if everything fine - I'll release v0.16 with this improvement.

Ping me if notice any weird behaviour.

@NeXTs NeXTs closed this as completed in 208b163 Mar 12, 2016
@NeXTs
Copy link
Owner

NeXTs commented Mar 12, 2016

verify_change option was removed.
It's now permanently enabled.

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

2 participants