7 ms·

> Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted.

> Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. That's way too strict. If I want to suggest moving a statement that's inside a loop to the outside, I don't want to waste my time writing microbenchmarks showing that it improves performance. If the suggestion is complicated or non-obvious, and it's in a really critical part of the code, then sure, write a benchmark. Outside of that though, it's rarely a good use of developer time.

bt848 7y ago
I'm not saying you should write code in a performance-oblivious fashion. I am saying that if you want to do something non-canonical, or you want an exception to style guide or other established guidelines, or if you write a comment in your code that says "This function is faster than std::foo", then you must have evidence. In the absence of evidence, adhere to normal rules. I came across a comment in google base libraries that said "This is faster because cache line is 32 bytes". It had been written by a very famous engineer, and it was even true in the days of the Pentium III processor. But at the time I found it it was not only false but the code as written was slower on modern CPUs than the shorter and totally obvious equivalent.
- rstuart4133 7y ago
  > I came across a comment in google base libraries that said "This is faster because cache line is 32 bytes". You would want to ban that sort of comment? If you see a piece of code written oddly, then someone telling you why they wrote it that way is very helpful. If later you have to refactor it then it's doubly nice to know the reason it was written that way doesn't apply any more. I suspect your problem is with pre-mature optimisation rather than commenting, and if so I imagine the majority of programmers would share your views. But if that is the case banning comments that make it plain something may have been pre-maturely optimised doesn't seem like a good way of solving the problem.
dxf 7y ago
You have to be careful though. The compiler can do a very good job of lifting statements (and many other optimizations), so it is generally a better use of developer time to try and write code that is clear than to write code that is believed (without evidence) to be more performant.
aspaceman 7y ago
I agree - most folks overestimate the effectiveness of microbenchmarks. I work in a field where microbenchmarks often feel useless: computer graphics. Often we have to consider the entire pipeline of work. Writing to a texture in Stage 3 may seem perfectly fine, but it could be thrashing the cache in Stage 4 when the texture is being read from. Benchmarking them separately misses this.
- dxf 7y ago
  You're right that many microbenchmarks can be noisy (and the environment you run them on can introduce more noise). But if you're attending cppcon, two engineers in Google's production toolchain organization are giving a talk on "Releasing C++ Toolchains Weekly in a 'Live at Head' World" where they'll discuss (among other things) how we use various microbenchmarks to catch performance regressions. https://sched.co/Sft4 https://sched.co/Sft4
  - aspaceman 7y ago
    Ah I’ll take a look!
nimblegorilla 7y ago
> If the suggestion is complicated or non-obvious, and it's in a really critical part of the code, then sure, write a benchmark. Outside of that though, it's rarely a good use of developer time. If it isn't worth your time to write a benchmark then it isn't worth my time to change the code. Everyone thinks their own performance tweaks are simple and obvious, but I've rarely seen measurable performance boosts from comments in a typical PR.
molodec 7y ago
I agree that "must" is too strict. If I replace linear search with binary search, or hashmap/hashset lookup I don't need to write a benchmark to prove it improves performance. There is math, logic, Big O analysis that allows to reason about performance and speed without microbenchmarks.
- bt848 7y ago
  If you did that to code in Google search you’d be required to run a special load test to prove you didn’t screw it up. Nobody should assume either of the things you just implied were obvious. In fact linear search is guaranteed to beat binary search for short vectors. O-complexity analysis is good for undergrads but the ONLY aspect of software performance that matters any more is cache behavior.
- BeetleB 7y ago
  bt848 already said it, but you picked a really poor example. In many real world scenarios, a linear search beats a binary search due to a lack of overhead. Along those lines, in C++, a vector very often performs better than a theoretically better data structure. I learned C++ fairly well from a very experienced guy in the company, and he said that you should always benchmark against a vector. I suspect if I was in his team and he were reviewing my code, he wouldn't let the code pass unless I had benchmarks that show whatever data set I picked is faster than a vector. He wasn't against other data structures - he just wanted proof they would perform better. In quite a few cases, they didn't.
  - jsnell 7y ago
    So now the requirement is not just to run a micro benchmark, but to run one with inputs that approximate the distribution of production inputs, along all possible axes. For many sorts of projects, this is totally unreasonable. It's easy to figure out how the code performs with a given input, it is much harder to figure out what the inputs really are like. In this particular case I'd expect the change to be motivated by profiling of the real production instances. And that makes it pretty obvious how the change should be evaluated. "We're spending more time than is reasonable in linear scans, so the worst case inputs must be worse than expected. Switch to a data structure more suited to large inputs, and see if CPU use improves in the next rollout."
Cthulhu_ 7y ago
I want to say that yes, if it's simple and straightforward enough or obvious then just do it - it should have been written like that in the first place. BUT I think a more important consideration is readability and clarity. Make it work, make it pretty, make it fast - in that order. If there was anything I learned from a Go course some time ago (Ultimate Go iirc) is that speed is a natural result of readable code. Plus if your code is good and well structured it becomes trivial to benchmark, identify and resolve any performance issues.

bt848 7y ago

I'm not saying you should write code in a performance-oblivious fashion. I am saying that if you want to do something non-canonical, or you want an exception to style guide or other established guidelines, or if you write a comment in your code that says "This function is faster than std::foo", then you must have evidence. In the absence of evidence, adhere to normal rules. I came across a comment in google base libraries that said "This is faster because cache line is 32 bytes". It had been written by a very famous engineer, and it was even true in the days of the Pentium III processor. But at the time I found it it was not only false but the code as written was slower on modern CPUs than the shorter and totally obvious equivalent.

rstuart4133 7y ago
> I came across a comment in google base libraries that said "This is faster because cache line is 32 bytes". You would want to ban that sort of comment? If you see a piece of code written oddly, then someone telling you why they wrote it that way is very helpful. If later you have to refactor it then it's doubly nice to know the reason it was written that way doesn't apply any more. I suspect your problem is with pre-mature optimisation rather than commenting, and if so I imagine the majority of programmers would share your views. But if that is the case banning comments that make it plain something may have been pre-maturely optimised doesn't seem like a good way of solving the problem.

dxf 7y ago

You have to be careful though. The compiler can do a very good job of lifting statements (and many other optimizations), so it is generally a better use of developer time to try and write code that is clear than to write code that is believed (without evidence) to be more performant.

aspaceman 7y ago

I agree - most folks overestimate the effectiveness of microbenchmarks. I work in a field where microbenchmarks often feel useless: computer graphics. Often we have to consider the entire pipeline of work. Writing to a texture in Stage 3 may seem perfectly fine, but it could be thrashing the cache in Stage 4 when the texture is being read from. Benchmarking them separately misses this.

dxf 7y ago
You're right that many microbenchmarks can be noisy (and the environment you run them on can introduce more noise). But if you're attending cppcon, two engineers in Google's production toolchain organization are giving a talk on "Releasing C++ Toolchains Weekly in a 'Live at Head' World" where they'll discuss (among other things) how we use various microbenchmarks to catch performance regressions. https://sched.co/Sft4 https://sched.co/Sft4
- aspaceman 7y ago
  Ah I’ll take a look!

nimblegorilla 7y ago

> If the suggestion is complicated or non-obvious, and it's in a really critical part of the code, then sure, write a benchmark. Outside of that though, it's rarely a good use of developer time. If it isn't worth your time to write a benchmark then it isn't worth my time to change the code. Everyone thinks their own performance tweaks are simple and obvious, but I've rarely seen measurable performance boosts from comments in a typical PR.

molodec 7y ago

I agree that "must" is too strict. If I replace linear search with binary search, or hashmap/hashset lookup I don't need to write a benchmark to prove it improves performance. There is math, logic, Big O analysis that allows to reason about performance and speed without microbenchmarks.

bt848 7y ago
If you did that to code in Google search you’d be required to run a special load test to prove you didn’t screw it up. Nobody should assume either of the things you just implied were obvious. In fact linear search is guaranteed to beat binary search for short vectors. O-complexity analysis is good for undergrads but the ONLY aspect of software performance that matters any more is cache behavior.
BeetleB 7y ago
bt848 already said it, but you picked a really poor example. In many real world scenarios, a linear search beats a binary search due to a lack of overhead. Along those lines, in C++, a vector very often performs better than a theoretically better data structure. I learned C++ fairly well from a very experienced guy in the company, and he said that you should always benchmark against a vector. I suspect if I was in his team and he were reviewing my code, he wouldn't let the code pass unless I had benchmarks that show whatever data set I picked is faster than a vector. He wasn't against other data structures - he just wanted proof they would perform better. In quite a few cases, they didn't.
- jsnell 7y ago
  So now the requirement is not just to run a micro benchmark, but to run one with inputs that approximate the distribution of production inputs, along all possible axes. For many sorts of projects, this is totally unreasonable. It's easy to figure out how the code performs with a given input, it is much harder to figure out what the inputs really are like. In this particular case I'd expect the change to be motivated by profiling of the real production instances. And that makes it pretty obvious how the change should be evaluated. "We're spending more time than is reasonable in linear scans, so the worst case inputs must be worse than expected. Switch to a data structure more suited to large inputs, and see if CPU use improves in the next rollout."

Cthulhu_ 7y ago

I want to say that yes, if it's simple and straightforward enough or obvious then just do it - it should have been written like that in the first place. BUT I think a more important consideration is readability and clarity. Make it work, make it pretty, make it fast - in that order. If there was anything I learned from a Go course some time ago (Ultimate Go iirc) is that speed is a natural result of readable code. Plus if your code is good and well structured it becomes trivial to benchmark, identify and resolve any performance issues.