20 ms·

But I was helping the compiler

72 points by aw1621107 6y ago

lilyball 6y ago
Why does adding the std::move() change the generated code though? It changes the prvalue to an xvalue, but std::move is equivalent to a static_cast, and shouldn’t both the prvalue and the xvalue versions end up calling the same std::array move constructor? Edit: I just took a look at the actual godbolt links. And it looks like the std::move version actually puts a call to std::move in the generated assembly. I’m kind of surprised this is actually a literal function call, I would have assumed std::move would be always inlined.
- rrss 6y ago
  > I’m kind of surprised this is actually a literal function call, I would have assumed std::move would be always inlined The godbolt links are without any optimization.
- edflsafoiewq 6y ago
  The version without std::move does not call a constructor because of copy elision. The get_display_pixels function constructs the disp_pixels object in place. In the version with std::move, it first copy-elides the function call into a temporary, then move-constructs the disp_pixels object from the temporary. ie as if auto tmp = emulator.get_display_pixels(); const auto disp_pixels = std::move(tmp); // move constructor called here // lifetime of tmp ends So the version with std::move is invoking an extra move-constructor and destructor call.
  - iainmerrick 6y ago
    But shouldn’t those extra calls be optimized away to nothing, if they don’t have any real side-effects? That’s what I would naively assume.
    - edflsafoiewq 6y ago
      I expect so, yes.
- aw1621107 6y ago
  I think the calls to std::move in those specific examples are due to optimization flags not being passed to the compiler invocations on godbolt. That being said, I've definitely heard of std::move in the wrong places preventing optimizations (e.g., std::move on a return statement can (will?) inhibit copy elison), but I'm not familiar enough with the finer details of C++ be able to tell you why.
notacoward 6y ago
This is the thing that always drives me nuts about C++: the compiler always trying to second-guess me, or me having to second-guess it. If I had wanted to spend time thinking about compiler internals I would have made that choice decades ago (after I actually did work on compilers a bit along with everything else). If I want low-level control to achieve best performance or efficiency, I'll just use C and be careful to avoid its deficiencies. Been there, done that, I'm actually pretty good at it. Then I'll link that with code written in a language that lets me express higher-level intent at a decent level of abstraction, which is impossible with the $#@! compiler in my face all the time.
- jfkrorkt 6y ago
  On the other hand, it's basically impossible to write in C something as fast as Eigen, because it's exactly the compiler that gets into your face which can optimize away the intermediate layers of abstractions, in a way that would require 10 times the amount of code in C.
  - notacoward 6y ago
    Yes, there are specialized libraries for many things, and some of them even force use of a specific language. So by all means people should use those languages for those things. There are some pretty good Fortran libraries out there too. But that doesn't mean Fortran should be adopted as a primary language for every kind of task within a large company, and the same reasoning applies to C++. A general-purpose language that's full of pitfalls or requires lots of boilerplate even for simple things is not really a general-purpose language IMO.
  - voldacar 6y ago
    Which also means that is basically impossible to read some eigen code and mentally visualize the compiled asm. Though there are certainly situations where that doesn't matter, it can be quite annoying.
    - ncmncm 6y ago
      It just takes some practice.
marta_morena_25 6y ago
You really don't need to read assembly for any but the most rare cases. What you need is a profiler to give you the hotspots and you need to stop prematurely optimizing on a line-by-line basis. Just stop it. Optimize O(n) performance (but also don't go wild, unless there is a business reason), keep your fingers off micro-optimizations, unless so indicated by the profiler and only if there is a business reason as well...
- gen220 6y ago
  Totally agreed. Getting comfortable with a profiler is a superpower. FWIW, this is also one of Rob Pike's rules of programming: > Rule 1. You can't tell where a program is going to spend its time. Bottlenecks occur in surprising places, so don't try to second guess and put in a speed hack until you've proven that's where the bottleneck is. It's implicit, but the "proven" part implies use of a profiler.
  - pkolaczk 6y ago
    This is a matter of experience. If I have a tight loop looping millions of times but I have a heap allocation inside, I can be 99% sure this would be slow, without looking at the profiler and if there is an easy way to avoid it without obfuscating the code (e.g simply taking it out of the loop, or using the stack instead of the heap), I'd just do it. Profiling is a waste of time in such cases. Also, bottlenecks happen in surprising places, and allowing slow/bad code just because the profiler doesn't scream about it on the development machine is a recipe for surprising performance issues in the future. The slow code might become a problem when a user has slightly different task for the program.
    - gen220 6y ago
      I agree with you, too, that experience helps. But I think the aphorism covers the case you’re describing too. It’s possible that your loop is slow, but if you’re working on a program of sufficient size and complexity, you simply won’t know if it’s the bottleneck without using a profiler. The purpose of the statement is to save you the trouble of 10xing performance of a function, so that it takes .1% of all execution time, instead of 1%. Do sufficiently complex programs, and without a profiler, you won’t really know if a loop is taking 1%, 10%, or 80%.
comex 6y ago
Two important points: - For std::array of uint8_t – an in-place container of a plain-old-data type – the move constructor and the copy constructor, if not optimized away, will do the exact same thing! Both boil down to copying the bytes from the old object to the new object. So there is never any benefit to calling std::move on such an object. This is different from out-of-line containers like std::vector, where the std::vector object itself consists only of a pointer and some lengths. In that case, the move constructor just copies the pointer and length values themselves and nulls them out in the original object, without copying the underlying data. But the copy constructor has to allocate a whole new heap buffer, copy the underlying data, and initialize the new vector object with that pointer. - The godbolt links are building without optimization enabled (without -O settings)! GCC apparently still performs (non-mandatory) NRVO in this case, probably because that optimization is implemented on the frontend rather than the backend, and the frontend tends to care less about -O settings. But GCC does not perform the usual suite of backend optimizations, including inlining and memcpy elimination. If optimizations were enabled, GCC might be able to optimize both versions into the same thing, depending on what actually happens in “// Do some computation”. It doesn’t make sense to make performance comparisons with optimization disabled, unless you’re trying to improve the performance of debug builds themselves.
- jfkrorkt 6y ago
  Am I right to say that using std::move is even a logical mistake here? The purpose of move is to "steal" the underlying memory, but if you do that, you leave the gfx variable from the class without backing. So basically you would need to reallocate it after every call to get_display_pixels. So the code only works because the author uses array, which is statically allocated and there is nothing to steal. It would break were they to switch to vector. Either the author doesn't understand what the purpose of move is, or if this is really their intent, it's a terrible design, since get_display_pixels looks like an idempotent getter function, but it's not.
  - foota 6y ago
    Move leaves the object in a valid but unspecified state. So you can continue to use the existing object.
tylerhou 6y ago
1) It's useless to reason about the performance of generated assembly unless you use -O{1,2,3}. Another commenter says that they're surprised std::move isn't inlined; that's because -O{1,2,3} isn't passed. 2) If you care that much about performance, you can avoid all copies by returning by reference in this case. 3) The author seems to misunderstand std::move — in this case, std::move can only pessimize performance and never optimize because the move constructor of std::array is the exact same as the copy constructor. For example, move constructors help performance for std::vector because the move constructor will copy the backing buffer's pointer instead of copying the entire backing buffer itself. Since std::array<T> is a thin wrapper around T[], the entire array is stored on the stack, and there is no possible optimization that a move could make — stack values must be copied (or otherwise initialized) in move constructors. In fact, since std::array is an aggregate type, there isn't even a user-defined move constructor (or copy constructor)! https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/array#L112 https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-...
- remar 6y ago
  Do you have a quick explanation or resource that explains why [1] is true? Just curious as I've wanted to get better at being able to understanding assembly output by gcc/clang but don't think I've come across this advice explicitly before.
  - tylerhou 6y ago
    I should maybe rephrase [1] to: if you're deciding between two different ways of writing the same function for performance reasons, you should use the optimized assembly output to decide (or the same compiler flags as what you use in production). There can be big differences between the unoptimized and optimized code: https://godbolt.org/z/fszhjj https://godbolt.org/z/fszhjj. Unoptimized assembly IS useful if you just want to learn or understand assembly itself.
jeffbee 6y ago
Depending on what is in the body of the function, I usually prefer to see an output iterator taken as a function parameter, instead of functions returning whole collections. In the article's style the caller is forced to deal with std::array even if that's not suitable at the call site. With an output iterator and a template function the caller gets to pick whatever suits the purpose best. Not having to think about how the output was allocated and deleted is just a bonus of this style.
knorker 6y ago
Never use std::move on an rvalue. That is always wrong, and shows a misunderstanding of std::move. std::move doesn't move. It should maybe have been called std::lvalue_to_rvalue.
- knorker 6y ago
  … and I meant to add: Which makes it clear that you should neverer pass an rvalue into std::lvalue_to_rvalue.
platinumrad 6y ago
In addition to the significant issues pointed out by comex and tylerhou, NRVO is not in play here as `gfx` is not a non-volatile object with automatic storage duration.
CyberRabbi 6y ago
You don’t need a copy here. A const reference to the internal array is all you need, and it’s faster.

72 points by aw1621107 6y ago

Why does adding the std::move() change the generated code though? It changes the prvalue to an xvalue, but std::move is equivalent to a static_cast, and shouldn’t both the prvalue and the xvalue versions end up calling the same std::array move constructor? Edit: I just took a look at the actual godbolt links. And it looks like the std::move version actually puts a call to std::move in the generated assembly. I’m kind of surprised this is actually a literal function call, I would have assumed std::move would be always inlined.

rrss 6y ago
> I’m kind of surprised this is actually a literal function call, I would have assumed std::move would be always inlined The godbolt links are without any optimization.
edflsafoiewq 6y ago
The version without std::move does not call a constructor because of copy elision. The get_display_pixels function constructs the disp_pixels object in place. In the version with std::move, it first copy-elides the function call into a temporary, then move-constructs the disp_pixels object from the temporary. ie as if auto tmp = emulator.get_display_pixels(); const auto disp_pixels = std::move(tmp); // move constructor called here // lifetime of tmp ends So the version with std::move is invoking an extra move-constructor and destructor call.
- iainmerrick 6y ago
  But shouldn’t those extra calls be optimized away to nothing, if they don’t have any real side-effects? That’s what I would naively assume.
  - edflsafoiewq 6y ago
    I expect so, yes.
aw1621107 6y ago
I think the calls to std::move in those specific examples are due to optimization flags not being passed to the compiler invocations on godbolt. That being said, I've definitely heard of std::move in the wrong places preventing optimizations (e.g., std::move on a return statement can (will?) inhibit copy elison), but I'm not familiar enough with the finer details of C++ be able to tell you why.

notacoward 6y ago

This is the thing that always drives me nuts about C++: the compiler always trying to second-guess me, or me having to second-guess it. If I had wanted to spend time thinking about compiler internals I would have made that choice decades ago (after I actually did work on compilers a bit along with everything else). If I want low-level control to achieve best performance or efficiency, I'll just use C and be careful to avoid its deficiencies. Been there, done that, I'm actually pretty good at it. Then I'll link that with code written in a language that lets me express higher-level intent at a decent level of abstraction, which is impossible with the $#@! compiler in my face all the time.

jfkrorkt 6y ago
On the other hand, it's basically impossible to write in C something as fast as Eigen, because it's exactly the compiler that gets into your face which can optimize away the intermediate layers of abstractions, in a way that would require 10 times the amount of code in C.
- notacoward 6y ago
  Yes, there are specialized libraries for many things, and some of them even force use of a specific language. So by all means people should use those languages for those things. There are some pretty good Fortran libraries out there too. But that doesn't mean Fortran should be adopted as a primary language for every kind of task within a large company, and the same reasoning applies to C++. A general-purpose language that's full of pitfalls or requires lots of boilerplate even for simple things is not really a general-purpose language IMO.
- voldacar 6y ago
  Which also means that is basically impossible to read some eigen code and mentally visualize the compiled asm. Though there are certainly situations where that doesn't matter, it can be quite annoying.
  - ncmncm 6y ago
    It just takes some practice.

marta_morena_25 6y ago

You really don't need to read assembly for any but the most rare cases. What you need is a profiler to give you the hotspots and you need to stop prematurely optimizing on a line-by-line basis. Just stop it. Optimize O(n) performance (but also don't go wild, unless there is a business reason), keep your fingers off micro-optimizations, unless so indicated by the profiler and only if there is a business reason as well...

gen220 6y ago
Totally agreed. Getting comfortable with a profiler is a superpower. FWIW, this is also one of Rob Pike's rules of programming: > Rule 1. You can't tell where a program is going to spend its time. Bottlenecks occur in surprising places, so don't try to second guess and put in a speed hack until you've proven that's where the bottleneck is. It's implicit, but the "proven" part implies use of a profiler.
- pkolaczk 6y ago
  This is a matter of experience. If I have a tight loop looping millions of times but I have a heap allocation inside, I can be 99% sure this would be slow, without looking at the profiler and if there is an easy way to avoid it without obfuscating the code (e.g simply taking it out of the loop, or using the stack instead of the heap), I'd just do it. Profiling is a waste of time in such cases. Also, bottlenecks happen in surprising places, and allowing slow/bad code just because the profiler doesn't scream about it on the development machine is a recipe for surprising performance issues in the future. The slow code might become a problem when a user has slightly different task for the program.
  - gen220 6y ago
    I agree with you, too, that experience helps. But I think the aphorism covers the case you’re describing too. It’s possible that your loop is slow, but if you’re working on a program of sufficient size and complexity, you simply won’t know if it’s the bottleneck without using a profiler. The purpose of the statement is to save you the trouble of 10xing performance of a function, so that it takes .1% of all execution time, instead of 1%. Do sufficiently complex programs, and without a profiler, you won’t really know if a loop is taking 1%, 10%, or 80%.

comex 6y ago

Two important points: - For std::array of uint8_t – an in-place container of a plain-old-data type – the move constructor and the copy constructor, if not optimized away, will do the exact same thing! Both boil down to copying the bytes from the old object to the new object. So there is never any benefit to calling std::move on such an object. This is different from out-of-line containers like std::vector, where the std::vector object itself consists only of a pointer and some lengths. In that case, the move constructor just copies the pointer and length values themselves and nulls them out in the original object, without copying the underlying data. But the copy constructor has to allocate a whole new heap buffer, copy the underlying data, and initialize the new vector object with that pointer. - The godbolt links are building without optimization enabled (without -O settings)! GCC apparently still performs (non-mandatory) NRVO in this case, probably because that optimization is implemented on the frontend rather than the backend, and the frontend tends to care less about -O settings. But GCC does not perform the usual suite of backend optimizations, including inlining and memcpy elimination. If optimizations were enabled, GCC might be able to optimize both versions into the same thing, depending on what actually happens in “// Do some computation”. It doesn’t make sense to make performance comparisons with optimization disabled, unless you’re trying to improve the performance of debug builds themselves.

jfkrorkt 6y ago
Am I right to say that using std::move is even a logical mistake here? The purpose of move is to "steal" the underlying memory, but if you do that, you leave the gfx variable from the class without backing. So basically you would need to reallocate it after every call to get_display_pixels. So the code only works because the author uses array, which is statically allocated and there is nothing to steal. It would break were they to switch to vector. Either the author doesn't understand what the purpose of move is, or if this is really their intent, it's a terrible design, since get_display_pixels looks like an idempotent getter function, but it's not.
- foota 6y ago
  Move leaves the object in a valid but unspecified state. So you can continue to use the existing object.

tylerhou 6y ago

1) It's useless to reason about the performance of generated assembly unless you use -O{1,2,3}. Another commenter says that they're surprised std::move isn't inlined; that's because -O{1,2,3} isn't passed. 2) If you care that much about performance, you can avoid all copies by returning by reference in this case. 3) The author seems to misunderstand std::move — in this case, std::move can only pessimize performance and never optimize because the move constructor of std::array is the exact same as the copy constructor. For example, move constructors help performance for std::vector because the move constructor will copy the backing buffer's pointer instead of copying the entire backing buffer itself. Since std::array<T> is a thin wrapper around T[], the entire array is stored on the stack, and there is no possible optimization that a move could make — stack values must be copied (or otherwise initialized) in move constructors. In fact, since std::array is an aggregate type, there isn't even a user-defined move constructor (or copy constructor)! https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/array#L112 https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-...

remar 6y ago
Do you have a quick explanation or resource that explains why [1] is true? Just curious as I've wanted to get better at being able to understanding assembly output by gcc/clang but don't think I've come across this advice explicitly before.
- tylerhou 6y ago
  I should maybe rephrase [1] to: if you're deciding between two different ways of writing the same function for performance reasons, you should use the optimized assembly output to decide (or the same compiler flags as what you use in production). There can be big differences between the unoptimized and optimized code: https://godbolt.org/z/fszhjj https://godbolt.org/z/fszhjj. Unoptimized assembly IS useful if you just want to learn or understand assembly itself.

jeffbee 6y ago

Depending on what is in the body of the function, I usually prefer to see an output iterator taken as a function parameter, instead of functions returning whole collections. In the article's style the caller is forced to deal with std::array even if that's not suitable at the call site. With an output iterator and a template function the caller gets to pick whatever suits the purpose best. Not having to think about how the output was allocated and deleted is just a bonus of this style.

knorker 6y ago

Never use std::move on an rvalue. That is always wrong, and shows a misunderstanding of std::move. std::move doesn't move. It should maybe have been called std::lvalue_to_rvalue.

knorker 6y ago
… and I meant to add: Which makes it clear that you should neverer pass an rvalue into std::lvalue_to_rvalue.

platinumrad 6y ago

In addition to the significant issues pointed out by comex and tylerhou, NRVO is not in play here as `gfx` is not a non-volatile object with automatic storage duration.

CyberRabbi 6y ago

You don’t need a copy here. A const reference to the internal array is all you need, and it’s faster.