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

Speed up StyleContextStack.autopush() for large tables #2733

Merged

Conversation

estanglerbm
Copy link

Speed up StyleContextStack.autopush() for large tables by skipping slower creation of styleOverrideObject and just using the item itself.

Three tests are updated to handle the change in behavior.

Implements #2732

@estanglerbm
Copy link
Author

Looks like the two build errors are build (not code) issues?

Error: Unable to find Node version '12.x' for platform darwin and architecture arm64.

@liborm85
Copy link
Collaborator

Error: Unable to find Node version '12.x' for platform darwin and architecture arm64.

It is error in GitHub Action, no problem.

According to your edits, you remove style properties filtering. This is unwanted behavior.

@estanglerbm
Copy link
Author

Right. The style properties filtering is really slow over many table rows. Other methods (reduce, destructuring, etc.) don't seem to speed it up. [Reduce is slower, destructuring same speed.]

StyleContextStack.autopush() is used only in StyleContextStack.auto(), and only to push items, do callback, and then pop those items. So this is about a tradeoff; seems like little benefit of filtering for this operation, and the speedup is significant (especially because DocMeasure.measureTable() ends up calling StyleContextStack.auto() twice per row, which is another issue).

I can't find any code that would end up enumerating the style context stack, and none of the callbacks appear to be under user control, so I don't think this change would really leak info. If this tradeoff is unacceptable, then this PR is not needed. Or perhaps a different StyleContextStack.auto(), or different measureNode(), is needed for table rows.

@liborm85
Copy link
Collaborator

I don't know if this filtering is necessary.

@liborm85 liborm85 merged commit c7caabb into bpampuch:0.2 Jul 30, 2024
13 of 15 checks passed
@liborm85
Copy link
Collaborator

It doesn't really seem to matter. Merged. Thanks.

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.

2 participants