Thursday, 1 March 2018

Firefox Media Playback Team Review Policy

Reviews form a central part of how we at Mozilla ensure engineering diligence. Prompt, yet thorough, reviews are also a critical component in maintaining team velocity and productivity. Reviews are also one of the primary ways that a distributed organization like Mozilla does its mentoring and development of team members.

So given how important reviews are, it pays to be deliberate about what you're aiming for.

The senior members of the Firefox Media Playback team met in Auckland in August 2016 to codify the roadmap, vision, and policy for the team, and and one of the things we agreed upon was our review policy.

The policy has served us well, as I think we've demonstrated with all we've achieved, so I'm sharing it here in the hope that it inspires others.
  • Having fast reviews is a core value of the media team.
  • Review should be complete by end of next business day.
  • One patch for one logical scope or change. Don't cram everything into one patch!
  • Do not fix a problem, fix the cause. Workarounds are typically bad. Look at the big picture and find the cause.
  • We should strive for a review to be clear. In all cases it should be clear what the next course of action is.
  • Reviews are there to keep bad code out of the tree.
  • Bad code tends to bring out bad reviews.
  • Commit message should describe what the commit does and why. It should describe the old bad behaviour, and the new good behaviour, and why the change needs to be made.
  • R+ means I don’t want to see it again. Maybe with comments that must be addressed before landing.
  • R- means I do want to see it again, with a list of things to fix.
  • R canceled means we’re not going to review this.
  • Anyone on the media team should be expected to complete a follow up bug.
  • It’s not OK for a reviewer to ask a test to be split out from a changeset, provided the test is related to the commit. By the time a patch gets to review, splitting the test out doesn’t create value, just stop-energy.
  • Review request. If response is slow, ping or email for a reminder, otherwise find another reviewer.
  • Don’t be afraid to ask when the review will come. The reply to “when” can be “is it urgent?”
  • Everyone should feel comfortable pointing out flaws/bugs as a “drive by”.
  • Give people as much responsibility as they can handle.
  • Reviewers should make it clear what they haven’t reviewed.
  • American English spelling, for comments and code.
  • Enforce Mozilla coding style, and encourage auto formatters, like `./mach clang-format`.
  • Use reviewboard. Except when you can’t, like security patches.

Friday, 12 January 2018

Not every bit of code you write needs to be optimal

It's easy to fall into the trap of obsessing about performance and try to micro-optimize every little detail in the code you're writing. Or reviewing for that matter. Most of the time, this just adds complexity and is a waste of effort.

If a piece of code only runs a few (or even a few hundred) times a second, a few nanoseconds per invocation won't make a significant difference. Chances are the performance wins you'll gain by micro optimizing such code won't show up on a profile.

Given that, what should you do instead? Code is read and edited much more than it is written, so optimize for readability, and maintainability.

If you find yourself wondering whether a piece of code is making your program slow, one of the first things you should do is fire up a profiler, and measure it. Or add telemetry to report how long your function takes in the wild. Then you can stop guessing, and start doing science.

If data shows that your code is slow, by all means optimize it. But if not, you can get more impact out of your time by directing your efforts elsewhere.