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

Add an index of refs to each destination for faster lookups #77

Merged
merged 1 commit into from May 26, 2022

Conversation

nschulzke
Copy link
Contributor

We were attempting to render a document that had thousands of cells in a massive table, and found that it took a very long time to render.

Profiling revealed that the bulk of the time was spent in running findElement in layout.js's append function. querySelector became pretty inefficient when the document got this large. I was able to reduce load times for one of our documents from 20s to 4s by building up an index of refs while appending nodes and then referencing that index later in findRef (in dom.js).

I thought I'd send this change upstream in case someone else would benefit from the performance. This version just stores the index directly on the DOM node, but I'd be happy to make any changes necessary to get this to fit in better with paged.js's conventions (or, obviously, if there are problems with this caching that I didn't anticipate).

@julientaq
Copy link
Collaborator

ho wow!
This is pretty cool!

I’m gonna have a look at that and bring that back to @fchasen .

We’ll be back to you soonish :)

Thanks a lot for sharing your solution!

@nschulzke
Copy link
Contributor Author

nschulzke commented May 19, 2022

On further troubleshooting, it turns out that the performance problems that this was meant to address are only a concern when there's very little text content. I was testing with completely empty rows as a simple benchmark, not realizing that that would mean that renderTo wouldn't check for a break until it had copied the entire table (after the last breakToken) into the new page.

This change still provides a marginal increase, but it only gives the major returns I was seeing in cases where there are a lot of empty nodes that clutter up the DOM without contributing to the length.

@julientaq
Copy link
Collaborator

I yeah, guess it makes sense.
But tables being table, they’re by definition full of empty very smalls cells full of data. (when they’re not used for layout :-p)

@fchasen fchasen merged commit 1627be4 into pagedjs:main May 26, 2022
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

Successfully merging this pull request may close these issues.

None yet

3 participants