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

Significant textLayer performance regression after PR 15722 #15844

Closed
Snuffleupagus opened this issue Dec 16, 2022 · 4 comments · Fixed by #15845
Closed

Significant textLayer performance regression after PR 15722 #15844

Snuffleupagus opened this issue Dec 16, 2022 · 4 comments · Fixed by #15845
Assignees

Comments

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Dec 16, 2022

Attach (recommended) or Link to PDF file here: https://github.com/mozilla/pdf.js/files/1522715/wuppertal_2012.pdf

Configuration:

Steps to reproduce the problem:
Open the referenced PDF document, and measure the time it takes for the textLayer to finish rendering.

What is the expected behavior? (add screenshot)
The textLayer should render in ~7 seconds (or better).

What went wrong? (add screenshot)
The textLayer takes ~140 seconds to render.

Note that if I add this.div.hidden = true after the following line performance is back to normal, although it's not clear to me if that's a correct/safe solution in general.
The reason that this regressed is presumably that we used to use a DocumentFragment for building the textLayer, but now we we're using a regular div which is immediately attached to the DOM thus causing lots of invalidation during layout.

/cc @calixteman

@calixteman
Copy link
Contributor

Considering:

this.show();

I think the hidden thing is an oblivion.

@calixteman
Copy link
Contributor

Very good catch !
On my workstation (which is supposed to be a good one): ~206s without the patch vs ~10s with.

@calixteman
Copy link
Contributor

Unrelated to this issue but do you think that it could be interesting to measure the strings in the worker ?

@Snuffleupagus
Copy link
Collaborator Author

Note that I found this issue when I noticed that zoom-performance was now extremely poor for that PDF document (zooming was never fast but now it was really bad).
Initially I figured that we somehow regressed general rendering performance, but couldn't understand how, so it took me a little while with both mozregression and git bisect to narrow down the offending patch. Then it took some more time to figure out why it regressed and how to possibly fix it...


Unrelated to this issue but do you think that it could be interesting to measure the strings in the worker ?

Maybe, if it measurably improves performance and isn't really complicated to implement. (I'm assuming that we'd use OffscreenCanvas, which unfortunately means that we'd still need to keep the old code in the src/display/text_layer.js file as a fallback.)

calixteman added a commit that referenced this issue Dec 16, 2022
[TextLayer] Hide the text layer when it's created in order to avoid reflows (fix #15844)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants