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

Improve performance of optional content parsing #17166

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

calixteman
Copy link
Contributor

No description provided.

@calixteman
Copy link
Contributor Author

@Snuffleupagus, I don't have a pdf for testing correctly that stuff but at least unit tests are ok locally.
A user shared a profile on the pdf.js channel which shew that we spend a lot of time in Array.includes so this patch should fix this issue.
Here's the profile:
Firefox 2023-10-24 12.17 profile.json.gz

@Snuffleupagus
Copy link
Collaborator

I don't have a pdf for testing correctly that stuff

That's really unfortunate!

but at least unit tests are ok locally.

What about the reference tests though, since that seems much more relevant given that optionalContent directly affects rendering?

  • First of all, please make sure that this passes all tests.
  • Secondly, please get the user to test this patch and confirm that it actually helps.

@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/4000d45d5dd12ff/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/dcc321c303c72b2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/4000d45d5dd12ff/output.txt

Total script time: 24.78 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 18
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/4000d45d5dd12ff/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/dcc321c303c72b2/output.txt

Total script time: 36.77 mins

  • Font tests: FAILED
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 20

Image differences available at: http://54.193.163.58:8877/dcc321c303c72b2/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/9dccddd7a186d74/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/9dccddd7a186d74/output.txt

Total script time: 1.43 mins

Published

src/core/catalog.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's land this since nothing breaks, and given that the profile shared on Matrix suggests a noticeable improvement.
This really seems like the best we can do without access to the PDF document in question.

r=me, thank you!

@calixteman calixteman merged commit b31e77d into mozilla:master Oct 25, 2023
3 checks passed
@calixteman calixteman deleted the improve_oc_parsing branch October 25, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants