-
-
Notifications
You must be signed in to change notification settings - Fork 377
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
Some optimizations #880
Some optimizations #880
Conversation
Hello @m-seker 👋 I'm your friendly neighborhood bot and would like to say thank you for So that you and other users can test your changes more quickly, If you make changes to your PR, i create a new link to your workflow artifacts. Best regards, |
{ | ||
memcpy(_pixels, other._pixels, (long) other._width * other._height * sizeof(Pixel_T)); | ||
_d_ptr = other._d_ptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This saves a bunch of copies
Image(const Image & other) : | ||
_width(other._width), | ||
_height(other._height), | ||
_pixels(new Pixel_T[other._width * other._height + 1]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This saves a bunch of heap allocations
fcf1e7c
to
6a49993
Compare
Here is your new link to your workflow artifacts. |
6a49993
to
ac20584
Compare
Here is your new link to your workflow artifacts. |
ac20584
to
689f26e
Compare
Here is your new link to your workflow artifacts. |
689f26e
to
7348d3a
Compare
Here is your new link to your workflow artifacts. |
7348d3a
to
1d5761a
Compare
Here is your new link to your workflow artifacts. |
61caec1
to
fe03a2d
Compare
Here is your new link to your workflow artifacts. |
fe03a2d
to
71f10d0
Compare
Here is your new link to your workflow artifacts. |
71f10d0
to
8c684ca
Compare
Here is your new link to your workflow artifacts. |
Merge? |
Summary
Use implicit sharing for Image data to eliminate redundant copies.
PS : A quick profiling says that we spend most of our time during ImageResampler::process(), it is mostly color conversion code. We can vectorize that code but portability is a concern. Maybe we can start with common SIMD instruction sets and fallback to normal for others. I did a quick test and my VM takes approximately 500us to resample the image. I tried implementing it with AVX2 and it is now around 50us.
Al in all, those number are no concern at all but optimization is always fun.
What kind of change does this PR introduce? (check at least one)
If changing the UI of web configuration, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing setups:
The PR fulfills these requirements:
Fixes: #xxx[,#xxx]
, where "xxx" is the issue number)If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.
Other information: