Comment by bt848 | Hacker News Reader

Comment by bt848 | Hacker News Reader

I'd add two things, from a decade of experience at Google:

Review code in this order: protocol buffers, unit tests, headers, implementation. It's common for a new employee to be an expert on C++ or Java or whatever languages but it's very uncommon to meet anyone who knows how to define a decent protocol message. The tests should give the reviewer a very clear idea of what's happening in the code (and this should be congruent with the description of the change), if not send back to author at this point. The headers should contain clear interfaces, types, and comments and should not contain anything that's not part of the API (when this is technically possible). Finally look in the CC file; at this point the reviewer should see things they were already expecting and no funny business.

Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. Wrong ideas about software performance abound, and even correct beliefs become incorrect with time, in which case the microbenchmarks are critical to evaluating the continued value of the code.

When sending a changelist for review, always clear all automated warnings or errors before wasting the reviewers' time. Nobody wants to see code that doesn't build, breaks a bunch of tests, doesn't lint, etc.

Re: performance claims, well put. I often see coding decisions and review claims made on untested or outdated facts. For Java specifically three most common things I run across are:

1) Decision to use some kind of 3rd party library that purportedly speeds up handling primitive types (Trove specifically. Almost never worth it, and it's extra painful because it doesn't plug in well into the rest of the standard library).

2) Using mutable objects to avoid the inefficiencies of allocation and copying with immutable patterns. Can lead to a brittle stateful design with hard to track down bugs.

3) Reusing serialization artifacts across the call stack to save copying/allocation, again like 2). Now you end up coupling the API interface to layers deep in the call stack making things harder to modify.

Also best review advice I got and follow: be the first reviewer yourself! Looking at a nicely formatted diff really lets you see your code in a new light, and usually I end up doing several iterations before I run out of things to tweak. Things like forgetting debug log statements or commented out code, or dev settings, as well as other easy but effective improvements.

> Also best review advice I got and follow: be the first reviewer yourself! Looking at a nicely formatted diff really lets you see your code in a new light, and usually I end up doing several iterations before I run out of things to tweak. Things like forgetting debug log statements or commented out code, or dev settings, as well as other easy but effective improvements.

I've been enjoying lots of comments in this thread, but unequivocally THIS. My company (for no particular reason) has two code review tools. Each team picks which one they like. Early in development I'll shelve a CL into the tool my team doesn't main, and just review and annotate diffs. It's a good place to keep TODOs and notes to myself.

Additionally, if I share it with my team, its clear that its "pre-review" quality and not to nitpick. "WIP" in the subject line helps, but being in an entirely different application really solidifies the point.

Then, when ready to go into real review, I've got a really good sense of how the CL looks as a diff, and where any remaining annotations need to be placed (that aren't truly a comment).

> 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.

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.

> 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.

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.

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

Ah I’ll take a look!

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.

> 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.

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.

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.

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.

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."

Yes, the size of the data matters, and linear search may be the right choice for many use cases and may outperform more advanced data structures. You learn it on the job as it is not a topic usually studied in universities. But I don't agree that we need to completely throw away theory, O complexity analysis where N - input size is the main variable. Microbenchmarks are hard to write correctly. Recreating the scenarios occurring in production is not always possible in the microbenchmark settings. You can write a microbenchmark that shows your code works great, but fails to perform in production because the input data is totally different. bt848 mentioned load test, which is not the same as microbenchmark. Load test that simulates production settings is a better tool to validate the optimizations.

>But I don't agree that we need to completely throw away theory, O complexity analysis where N - input size is the main variable.

No one here is agreeing with that.

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.

> When sending a changelist for review, always clear all automated warnings or errors before wasting the reviewers' time. Nobody wants to see code that doesn't build, breaks a bunch of tests, doesn't lint, etc.

Generally true, but not when prototyping. As a reviewer, I would like to see the general idea before spending time on cleaning up. If the grand design is wrong, time spent on cleaning up would be a waste.

Those are an oddly specific and narrowly focused set of recommendations to add to a set of guidelines that are largely very general and apply to a broad range of circumstances..

Sorry. I was responding mostly for this audience to the section about handling pushback. One common form of pushback I experienced was from authors who wanted an exception on performance grounds. These people were almost universally mislead, so I honed a specialized skill of disabusing wayward C++ authors of naive beliefs about performance.

No it's very practical advice that I use day to day as well. Basically check the interface first. Implementations can be fixed but contracts are harder to change when clients depend on it.

The review should simply be gated on passing the automated bits (tests/lint/warnings/style checkers whatever).

Reply on news.ycombinator.com