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.

Sign up for GitHub

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

Jump to bottom

Improve performance of shared/utils.js::intersect (bug 1135277) #14784

Merged
merged 1 commit into from
Apr 15, 2022
Merged

Improve performance of shared/utils.js::intersect (bug 1135277) #14784

merged 1 commit into from
Apr 15, 2022

Conversation

calixteman
Copy link
Contributor

  • avoid to call normalizeRect which clones the rectangles: it's useless
    and time consuming;
  • in profiling the pdf in bug 1135277, the time spent in intersect drops
    from ~1s to ~30ms.

@marco-c marco-c changed the title Improve performance of shared/utils.js::intersect Improve performance of shared/utils.js::intersect (bug 1135277) Apr 15, 2022
@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/2720d3a4b27359c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/53d3b527f45c211/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/2720d3a4b27359c/output.txt

Total script time: 25.29 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/53d3b527f45c211/output.txt

Total script time: 26.60 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: Passed

Snuffleupagus
Snuffleupagus approved these changes Apr 15, 2022
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment •
edited

Choose a reason for hiding this comment

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

r=me, since in addition to this being more efficient (fewer function calls and allocations) it's also shorter and easier to understand; thank you!

src/shared/util.js Outdated
Comment on lines 833 to 834
// [a, b] inter [c d] != 0 <=> c <= b && d >= a
// [a, b] inter [c d] = [max(a, c), min(b, d)] with max <= min.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might just be a question about the formatting here, but I'm sorry to say that I'm actually finding this comment slightly less clear than just carefully reading the code directly :-)

@calixteman
Improve performance of shared/utils.js::intersect
7501fe6 
- avoid to call normalizeRect which clones the rectangles: it's useless
  and time consuming;
- in profiling the pdf in bug 1135277, the time spent in intersect drops
  from ~1s to ~30ms.
@calixteman calixteman merged commit 681a9b8 into mozilla:master Apr 15, 2022
@calixteman calixteman deleted the intersect branch April 15, 2022 20:38
@calixteman calixteman mentioned this pull request Apr 22, 2022
Closed
zzeleznick added a commit to zzeleznick/pdf2json that referenced this pull request Mar 10, 2023
@zzeleznick
use patch from upstream mozilla/pdf.js#14784
0c8bb2b
@zzeleznick zzeleznick mentioned this pull request Mar 10, 2023
Merged
gayanMatch pushed a commit to gayanMatch/convert-pdf-json that referenced this pull request Jun 10, 2023
@littleMatch03
use patch from upstream mozilla/pdf.js#14784
f26686b
TadekDev added a commit to TadekDev/Pdf2Json that referenced this pull request Oct 24, 2023
@TadekDev
use patch from upstream mozilla/pdf.js#14784
207d687
modesty pushed a commit to modesty/pdf2json that referenced this pull request Nov 10, 2023
@zzeleznick
use patch from upstream mozilla/pdf.js#14784
8b9d66f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
core performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@calixteman @Snuffleupagus @pdfjsbot