3

In Visual C++ 2013, it is legal to assign a temporary value to a const reference, for example

const int& a = 1;

In a large software project, I now stumbled upon a single line of code that took around 10 ms to execute:

const std::vector<Item>& desc = (hasItems) ? instance->getItems() : std::vector<Item>();

Item is a struct with around 20 bytes, instance->getItems() returns a const std::vector< Item>&. I did not see any reason why this line should take longer than an instant to execute, so I played around a bit and found out that

const std::vector<Item>& desc = std::vector<Item>();

also takes 10 ms, while

std::vector<Item> items;
const std::vector<Item>& desc = items;

is near-instant, i.e. 0.00.. ms, as you would expect.

The thing is, I can reliably reproduce this issue in the existing, complex software project, but in any condensed minimal example, both versions run equally fast. I have tested all of this in a release build with optimizations enabled. Is there any chance that one of these optimization makes this exception to assign a value to a const reference really slow? I would really like to know what causes this behavior. Does anyone have a clue what causes this?

Following up, is there a warning for assigning a value to a const reference that I accidentally disabled? If so, what is its id? It would be handy if the compiler could point out other places in the code where this happens.

Edit: The timings were done with a simple CPU clock right before and right after the code of interest, divided by the clock cycles per second. It's not too accurate, but gives you a general idea of the difference between 10.something and 0.something. And it's not fluctuating in any way, it is perfectly reproducible in this specific example. There is a significant difference in execution time.

mOfl
  • 478
  • 3
  • 14
  • 4
    It is legal (and actually quite a useful feature) in C++ to bind a temporary variable to const reference, so there is no warning for that: https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/ – Zdeslav Vojkovic Jul 08 '16 at 08:24
  • 1
    Side note: that's not an assignment. – molbdnilo Jul 08 '16 at 08:25
  • const reference holds temporary object as long as the reference is alive, so it is legal – AnatolyS Jul 08 '16 at 08:33
  • The usage of the const reference would not be my first candidate for the performance issue. instance->getItems() could return not a reference but a copy of vector, or being a virtual function could also bring a little bit of overhead, or just do more than just returning the reference. – ead Jul 08 '16 at 08:37
  • 4
    How did you measure that this line takes 10ms? – MikeMB Jul 08 '16 at 08:44
  • @PcAF that refers to vector assignment, because assignment operator of `std::vector` is not called. Only the reference is bound to the instance. – Zdeslav Vojkovic Jul 08 '16 at 09:05
  • 2
    I believe that the measurement is broken. 10ms is a lot of time and I believe it is related to precision of your timing metod (e.g. GetTickCount precision varies up to 16 ms or more). If you compare generated assembly code you will see exactly what happens, and I believe there will not be much difference – Zdeslav Vojkovic Jul 08 '16 at 09:08
  • @ZdeslavVojkovic I thought that he is trying to say that it's not assignment to reference, but call to copy constructor of `vector` (which is non-sense to me), anyway term `assignment` is unclear in this context. – PcAF Jul 08 '16 at 09:10
  • @PcAF my understanding was just opposite :) I agree – Zdeslav Vojkovic Jul 08 '16 at 09:12
  • 1
    @PcAF Because it's an initialisation. – molbdnilo Jul 08 '16 at 09:28

1 Answers1

2
const std::vector<Item>& desc =
      (hasItems) ? instance->getItems() : std::vector<Item>();

You might not expect it, but this line makes a copy of the vector that the return value of getItems refers to.

Formally, the rules of type coercion performed by conditional operator conspire to make it so. Practically speaking, the compiler has no choice: when a reference is bound to a temporary, the compiler needs to generate code to destroy that temporary when the reference eventually goes out of scope - but that would get tricky if the same reference could be bound to a temporary sometimes and to an lvalue other times. It is precisely to avoid this situation that the rules of type coercion are written to ensure that a temporary is created on both branches of the conditional in your case.

Something like this should help:

std::vector<Item> blank;
const std::vector<Item>& desc =
      (hasItems) ? instance->getItems() : blank;

Or, I suppose, you could design getItems() to return a reference to a valid empty vector when it has no items, and not require callers to jump though hoops. Keeping a static always-empty vector variable for that purpose is not unreasonable.

Igor Tandetnik
  • 50,461
  • 4
  • 56
  • 85
  • @Lightness Well, the vector could be very large and/or copying `Item` could be very expensive. But yes, most likely the clock being used simply has a resolution of 10ms. – Igor Tandetnik Jul 08 '16 at 14:46
  • @Ig​​​​​​​​​​​​or: Apparently `Item` is "a struct with around 20 bytes" :P although granted that says nothing about what it does on copy construction, and we don't know how many there are :) – Lightness Races in Orbit Jul 08 '16 at 14:48