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

Optimize trimCanvas #8920

Merged
merged 3 commits into from Dec 4, 2022
Merged

Optimize trimCanvas #8920

merged 3 commits into from Dec 4, 2022

Conversation

SuperSodaSea
Copy link
Member

@SuperSodaSea SuperSodaSea commented Dec 2, 2022

Description of change

Optimize trimCanvas, using the implementation in https://gist.github.com/timdown/021d9c8f2aabc7092df564996f5afbbf. Also a new test for trimming empty canvas is added.

Closes #8815.

Pre-Merge Checklist

  • Tests and/or benchmarks are included
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

Comment on lines 18 to 32
expect(trimmedImageData.width).toEqual(9);
expect(trimmedImageData.width).toEqual(10);
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this is a bug in the old implementation. Since it is drawn by context.fillRect(10, 20, 10, 5), the width should be 10.

if (bound.right === null)
{
bound.right = x + 1;
}
else if (bound.right < x)
{
bound.right = x + 1;
}

Should be bound.right < x + 1 here in the old implementation.

Anyway, with the new implementation in this PR, we can get the correct width.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good point!

@SuperSodaSea SuperSodaSea added this to the v7.1.0 milestone Dec 2, 2022
@bigtimebuddy
Copy link
Member

This is awesome. Would be nice to see a performance comparison? How much faster is this?

@SuperSodaSea
Copy link
Member Author

It would be roughly (all pixels) / (trimmed pixels) times faster in my opinion. Since it won't be much empty space when trimming text, it can be a huge boost. Is there a good tool / library for making a benchmark?

@bigtimebuddy
Copy link
Member

I don't know something off-hand. There's a benchmark npm package you could try? I'm not looking for anything that rigorous, mostly to understand how much faster relative to current.

@bigtimebuddy
Copy link
Member

bigtimebuddy commented Dec 4, 2022

Here's a quick performance comparison:

v7.0.4: https://jsfiddle.net/bigtimebuddy/04vm18x6/ (15.038s)
PR: https://jsfiddle.net/bigtimebuddy/tqxbdfcg/ (14.206s)

This doesn't seem like a huge performance improvement (<10%). Could you run it and see if you see something similar?

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

This works, it's less code, and you fixed the off-by-one error. LGTM.

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label Dec 4, 2022
@SuperSodaSea
Copy link
Member Author

@bigtimebuddy Similar result here. However, using ctx.rect(10, 10, 780, 580) I got 37.21s vs 22.989s, ~30% speed up. The fewer pixels are trimmed, the greater the speed improvement. The remaining time should be the cost of getImageData.

@bigtimebuddy bigtimebuddy merged commit a984e4c into dev Dec 4, 2022
@bigtimebuddy bigtimebuddy deleted the chore/optimize-trim-canvas branch December 4, 2022 12:18
bigtimebuddy pushed a commit that referenced this pull request Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

trimCanvas: use better algorithm
2 participants