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

Reduce memory use and improve perfs when computing the bounding box of a bezier curve (bug 1875547) #17574

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Reduce memory use and improve perfs when computing the bounding box of a bezier curve (bug 1875547) #17574

merged 1 commit into from
Jan 25, 2024

Conversation

calixteman
Copy link
Contributor

It isn't really a fix for the mentioned bug but it slightly improve things. In reducing the memory use, the time spent in the GC is reduced either.
The algorithm to compute the bounding box is the same as before but it has just been rewritten to be more efficient.

@calixteman
Copy link
Contributor Author

calixteman commented Jan 24, 2024 •
edited
Loading

@Snuffleupagus, in the failing tests (I'm fixing) the line number where it occured is very wrong:

2) annotation InkAnnotation should render an added Ink annotation for printing
  Message:
    Expected $[1] = 2 to equal 1.
    Expected $[2] = 1 to equal 2.
  Stack:
        at <Jasmine>
        at UserContext.<anonymous> (file:///home/runner/work/pdf.js/pdf.js/build/lib-legacy/test/unit/annotation_spec.js:3052:38)

I don't remember having such issues in the past, could it be related to #17563 ?

@Snuffleupagus
Copy link
Collaborator

I don't remember having such issues in the past, could it be related to #17563 ?

No idea unfortunately. /cc @nicolo-ribaudo

Snuffleupagus
Snuffleupagus reviewed Jan 24, 2024
View reviewed changes
src/shared/util.js Show resolved Hide resolved
src/shared/util.js Outdated Show resolved Hide resolved
Snuffleupagus
Snuffleupagus previously approved these changes Jan 24, 2024
View reviewed changes
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.

r=me, with one more comment and all tests passing; thank you!

src/shared/util.js Show resolved Hide resolved
@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/56154ab2747f71b/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/6cb1610e96b758e/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/56154ab2747f71b/output.txt

Total script time: 24.73 mins

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

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

@Snuffleupagus Snuffleupagus dismissed their stale review January 24, 2024 22:06

Lots of ref-test failures.

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/6cb1610e96b758e/output.txt

Total script time: 40.90 mins

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

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

@calixteman
Reduce memory use and improve perfs when computing the bounding box o…
7f2428a 
…f a bezier curve (bug 1875547)

It isn't really a fix for the mentioned bug but it slightly improve things.
In reducing the memory use, the time spent in the GC is reduced either.
The algorithm to compute the bounding box is the same as before but it has just
been rewritten to be more efficient.
@calixteman
Copy link
Contributor Author

/botio test

@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/5d27a2b928ced57/output.txt

@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/c2dcfbe0fa6767b/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/c2dcfbe0fa6767b/output.txt

Total script time: 24.82 mins

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

Image differences available at: http://54.241.84.105:8877/c2dcfbe0fa6767b/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/5d27a2b928ced57/output.txt

Total script time: 39.55 mins

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

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

@calixteman calixteman merged commit bf92360 into mozilla:master Jan 25, 2024
9 checks passed
@calixteman calixteman deleted the improve_bezier_bbox branch January 25, 2024 09:07
@calixteman
Copy link
Contributor Author

I don't remember having such issues in the past, could it be related to #17563 ?

No idea unfortunately. /cc @nicolo-ribaudo

@nicolo-ribaudo ping ?

@timvandermeij
Copy link
Contributor

Wasn't that bug eventually filed in #17589 and fixed shortly after in #17596?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus left review comments

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

Successfully merging this pull request may close these issues.
4 participants
@calixteman @Snuffleupagus @moz-tools-bot @timvandermeij