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

Image::flipVertically(), Image::flipHorizontally() optimizations. #555

Merged
merged 1 commit into from Mar 30, 2014

Conversation

Projects
None yet
3 participants
@varnie
Contributor

varnie commented Mar 18, 2014

Hi.
I have made a few optimizations regarding these methods. Now they definitely perform better, but I kind of agree these're the micro-optimizations thus it is up to you whether or not to accept that.

Anyway, I benchmarked these methods using big PNG image (2048x2048px):

#include <SFML/Graphics.hpp>
#include <ctime>
#include <iostream>

int main() {

    sf::Image img;
    int num = 1000, i;

    if (img.loadFromFile("test_big_img.png")) {
    sf::Clock clock;
    for (i = 0; i < num; ++i) {
        //img.flipHorizontally();
        img.flipVertically();
    }
    sf::Time elapsed = clock.getElapsedTime();
    std::cout << "flipVertically: iterations: " << i << " total time: " << elapsed.asSeconds() << " average: " << elapsed.asSeconds() / num << std::endl;
    }

    return 0;
}

and here're the results:
//g++ -O2 -o bench ./bench.cpp -L/home/varnie/thrash/SFML-2.1/lib -lsfml-graphics -lsfml-window -lsfml-system

//current version: flipHorizontally: iterations: 1000 total time: 32.5636 average: 0.0325636
//new version : flipHorizontally: iterations: 1000 total time: 13.855 average: 0.013855

//current version: flipVertically: iterations: 1000 total time: 31.548 average: 0.031548
//new version : flipVertically: iterations: 1000 total time: 8.9526 average: 0.0089526

I have GCC version 4.7.2

The image I used is located here: https://www.dropbox.com/s/f6co1urplxtgatm/test_big_img.png

Feel free to code-review my changes. Thank you.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 18, 2014

Member

It looks good to me. And I wouldn't call that "micro" optimization.

However I don't think we will merge your code, the style differs too much from the SFML conventions, and there are things that can be improved (use std::swap or even std::swap_range in Image::flipHorizontally() instead of all these assignments).

I would also have written the benchmark differently: usually it is more robust/relevant to measure the whole execution time and divide by the number of iterations, rather than accumulating the tiny execution time of single calls.

Member

LaurentGomila commented Mar 18, 2014

It looks good to me. And I wouldn't call that "micro" optimization.

However I don't think we will merge your code, the style differs too much from the SFML conventions, and there are things that can be improved (use std::swap or even std::swap_range in Image::flipHorizontally() instead of all these assignments).

I would also have written the benchmark differently: usually it is more robust/relevant to measure the whole execution time and divide by the number of iterations, rather than accumulating the tiny execution time of single calls.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 18, 2014

Member

Here are some concrete style tips, in addition to Laurent's suggestions:

  • Declare variables locally, not before they are initialized. It doesn't make sense to "reuse" integers or iterators, it won't make a difference.
  • Use spaces between operators -- in some lines the only change is a removal of whitespace
  • In general, keep the changes to a minimum. For example, don't rename source/dest to src/dst.
  • The source could be a const_iterator.
  • Maybe Laurent can tell you the policy regarding 2 variables in the for loop initialization statement.

(And I'm not sure how easily the compiler could optimize the half size if it's part of the for loop's update statement, since m_size is a member and not a local variable and thus has a relatively wide scope. Now that is a micro-optimization.)

Member

Bromeon commented Mar 18, 2014

Here are some concrete style tips, in addition to Laurent's suggestions:

  • Declare variables locally, not before they are initialized. It doesn't make sense to "reuse" integers or iterators, it won't make a difference.
  • Use spaces between operators -- in some lines the only change is a removal of whitespace
  • In general, keep the changes to a minimum. For example, don't rename source/dest to src/dst.
  • The source could be a const_iterator.
  • Maybe Laurent can tell you the policy regarding 2 variables in the for loop initialization statement.

(And I'm not sure how easily the compiler could optimize the half size if it's part of the for loop's update statement, since m_size is a member and not a local variable and thus has a relatively wide scope. Now that is a micro-optimization.)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 18, 2014

Member

Maybe Laurent can tell you the policy regarding 2 variables in the for loop initialization statement.

Yep. I never do it (it quickly becomes hardly readable).

Here I would not create a variable just for that nano-optimization. In case you really want it, I'd define it before the loop.

Member

LaurentGomila commented Mar 18, 2014

Maybe Laurent can tell you the policy regarding 2 variables in the for loop initialization statement.

Yep. I never do it (it quickly becomes hardly readable).

Here I would not create a variable just for that nano-optimization. In case you really want it, I'd define it before the loop.

@varnie

This comment has been minimized.

Show comment
Hide comment
@varnie

varnie Mar 19, 2014

Contributor

Okay, I restructured it according to your suggestions. Are you interested in it? I might do a clean pull request. One notice: "The source could be a const_iterator." - no, it could not, I checked it.

Contributor

varnie commented Mar 19, 2014

Okay, I restructured it according to your suggestions. Are you interested in it? I might do a clean pull request. One notice: "The source could be a const_iterator." - no, it could not, I checked it.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 19, 2014

Member

Ah of course, you swap it :)

Where are your commits gone? You should push a single commit with all the changes in this pull request (do not open another one).

Member

Bromeon commented Mar 19, 2014

Ah of course, you swap it :)

Where are your commits gone? You should push a single commit with all the changes in this pull request (do not open another one).

@varnie

This comment has been minimized.

Show comment
Hide comment
@varnie

varnie Mar 19, 2014

Contributor

yes, I deleted them a few hours ago.
ok, now I made a fresh commit and changed the benchmarks snippet above (according to your suggestions about measuring the whole program run).

Contributor

varnie commented Mar 19, 2014

yes, I deleted them a few hours ago.
ok, now I made a fresh commit and changed the benchmarks snippet above (according to your suggestions about measuring the whole program run).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 19, 2014

Member

You shouldn't include the image loading into the measured time.

Member

LaurentGomila commented Mar 19, 2014

You shouldn't include the image loading into the measured time.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 19, 2014

Member

And even though it might not be relevant in that order of magnitude, you should generally prefer high-precision timers for profiling. The std::clock() function is not really precise. But SFML has sf::Clock which is also easier to use. But anyway, don't waste too much time with the benchmark, it's for sure an improvement (and if I find some time I could also have a look with a real profiler) ;)

The commit itself looks mostly good, but there are two { that should be on a separate line :P

Something else that just came to my mind: since the semantics of source/dest don't apply anymore, because you swap instead of copy, might it be worthwhile to rename the iterators? Something like left/right and top/bottom or upper/lower, better suggestions?

Member

Bromeon commented Mar 19, 2014

And even though it might not be relevant in that order of magnitude, you should generally prefer high-precision timers for profiling. The std::clock() function is not really precise. But SFML has sf::Clock which is also easier to use. But anyway, don't waste too much time with the benchmark, it's for sure an improvement (and if I find some time I could also have a look with a real profiler) ;)

The commit itself looks mostly good, but there are two { that should be on a separate line :P

Something else that just came to my mind: since the semantics of source/dest don't apply anymore, because you swap instead of copy, might it be worthwhile to rename the iterators? Something like left/right and top/bottom or upper/lower, better suggestions?

@varnie

This comment has been minimized.

Show comment
Hide comment
@varnie

varnie Mar 19, 2014

Contributor

Ouch, right, sorry. I am more confident with "One True Bracing Style" coding style. No problem, I'll fix that thingie and update the iterators' names.
I am sorry, but I am confused with the benchmarks code: earlier I've had the version you are talking about (well, almost, with std::clock), but Laurent said "it is more robust/relevant to measure the whole execution time and divide by the number of iterations, rather than accumulating the tiny execution time of single calls." and I changed it appropriately.

Load an image once and then perform N flips, is this what you mean?

So, how should it look like?

Contributor

varnie commented Mar 19, 2014

Ouch, right, sorry. I am more confident with "One True Bracing Style" coding style. No problem, I'll fix that thingie and update the iterators' names.
I am sorry, but I am confused with the benchmarks code: earlier I've had the version you are talking about (well, almost, with std::clock), but Laurent said "it is more robust/relevant to measure the whole execution time and divide by the number of iterations, rather than accumulating the tiny execution time of single calls." and I changed it appropriately.

Load an image once and then perform N flips, is this what you mean?

So, how should it look like?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 19, 2014

Member

Load an image once and then perform N flips, is this what you mean?

Yes. Don't benchmark unrelated operations such as loading the image, it's irrelevant. But measure the total time of your benchmark iterations to avoid accumulating timing imprecisions.

load image
start clock
for (num_iterations)
    flip image
end clock
display measured time / num_iterations

This way you don't even need a super precise clock since the measured time span is rather long.

Member

LaurentGomila commented Mar 19, 2014

Load an image once and then perform N flips, is this what you mean?

Yes. Don't benchmark unrelated operations such as loading the image, it's irrelevant. But measure the total time of your benchmark iterations to avoid accumulating timing imprecisions.

load image
start clock
for (num_iterations)
    flip image
end clock
display measured time / num_iterations

This way you don't even need a super precise clock since the measured time span is rather long.

@varnie

This comment has been minimized.

Show comment
Hide comment
@varnie

varnie Mar 20, 2014

Contributor

Please review the last commit. Hope it is truly last.

Contributor

varnie commented Mar 20, 2014

Please review the last commit. Hope it is truly last.

@Bromeon Bromeon added this to the 2.2 milestone Mar 30, 2014

Bromeon added a commit that referenced this pull request Mar 30, 2014

Merge pull request #555 from varnie/master
Image::flipVertically(), Image::flipHorizontally() optimizations

@Bromeon Bromeon merged commit e9334db into SFML:master Mar 30, 2014

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 30, 2014

Member

Hope it is truly last.

It is, thanks :)

Member

Bromeon commented Mar 30, 2014

Hope it is truly last.

It is, thanks :)

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