Code Review Velocity — The Need for Speed

A knuckle dragger engineer’s take on current practices and research related to code reviews

TL;DR

• Smaller code reviews do not end up being accepted or merged faster.
• A significant amount of time is wasted by code reviews just “sitting there idle,” waiting for an engineer to push a button to commit and merge the changes.
• When measuring and optimizing code velocity, focus on time-to-merge, not time-to-first-response or time-to-accept.
• Context is everything — don’t use an overall umbrella of a project or a company to quantity your code review metrics. Details matter!
• Want to have an impact and voice on current code review practices? Take our survey!

What metrics related to code reviews matter?

Dave Cutler, an engineer extraordinaire, used to say, “When all is said and done, much more is said than done.” Talk is cheap. Ideas are plentiful. Everyone thinks they are visionary. Blockchain, crypto, Web3. More blockchain.

However, at some point, the rubber hits the road, and the unpleasantness of reality will rise to its ugly head. Code velocity, or the speed with which code changes are reviewed and merged, may decide the fate of the company, project, or your career.

The design may be complete, the code has already undergone rigorous testing, the integration is “almost done,” and the changes are about to land “really-really soon.” But something is not quite right.

What should we measure or optimize? For code reviews, the typical period that most of the existing software engineering (SE) research measures is the time from when an engineer thinks something is ready to be reviewed until someone accepts the changes (time-to-accept).

However, any practicing engineer should know that accepting changes is an intermediate step on a long journey of “getting your changes in.”

Mark Hanna (played by Matthew McConaughey) from “The Wolf of Wall Street” would’ve made an excellent cynical software engineer:

Fugayzi, fugazi. It’s a whazy. It’s a woozie. It’s fairy dust. It doesn’t exist. It’s never landed. It is no matter. It’s not on the elemental chart. It’s not fucking real.

In practical terms, what matters the most is when the new code is finally integrated into an existing code base, and other engineers can use it. We call that period time-to-merge. So, why is most of the current research focused on the Holy Grail of acceptance and not when changes are merged to the destination branch?

In the movie “Ronin,” the protagonist Sam, played by Robert De Niro, says, “The only thing is that the map, the map is not the territory.”

We experience a similar disconnect in findings and research about code velocity because most SE researchers don’t have experience with complex industrial CI pipelines. Yes, the changes may be accepted in 23 minutes. That’s excellent. However, they may take the next three days to travel the gauntlet of pre-validation checks and repeatedly fail to build because of all kinds of unexpected issues. In addition, the OSS model, where sometimes git push is all that’s needed to merge your changes, is projecting the simplified worldview that SE researchers tend to accept.

Do small changes really merge faster?

Code reviews serve multiple purposes: detecting defects before the code is committed, spreading knowledge across the organization, seeking feedback about the proposed design, etc.

From a practical point of view, the quintessential question (well, at least in FAANG) is how do we speed up the entire process? How do we reduce the time between when code changes are submitted for review and when they reach their destination branch?

There are literally hundreds of factors that influence the time-to-merge. Everything from the availability of reviewers to the state of the CI infrastructure to parallel business priorities to the programming language involved to the identity of the code’s author, etc. etc. etc.

Few of these factors can be controlled by an engineer proposing the changes. Those that can are all related to changes themselves: complexity of changes, size (e.g., number of files, SLOC), and quality (e.g., readability, the amount of testing that went into code).

But can we influence the time-to-merge even by attempting to manipulate these factors? Is there a relationship between the size of code changes and the time it takes to accept or merge them? Can we somehow game the system?

Wouldn’t it be cool if we could clearly establish an “ideal range” for the code reviews, or be able to give guidance to engineers based on objective data that states, “if you scope your changes this way, then quite likely they will be merged faster?”

We looked at 845,316 pull requests from GitHub and a total of 401,790 code reviews from both Gerrit and Phabricator to find the answer to this. Our MSR 2022 paper “Do Small Code Changes Merge Faster? A Multi-Language Empirical Investigation” (arXiv copy here) says that there is no relationship between the size of code changes and both time-to-accept and time-to-merge.

Below is one of the scatterplots generated based on 1,000 randomly picked pull requests for the pull request size in SLOC and time-to-merge.

The process of generating the scatterplots can be repeated ad infinitum with a random selection of code review samples. The results are the same.

Is there low-hanging fruit to increase code velocity?

We’ll assume that if there’s a choice between getting your changes accepted and merged now or later, most engineers will choose now. However, before we dive into optimizing our CI infrastructure, adding more memory to the build servers, rewriting the file system (because it’s an interesting project and someone may actually fund it ;-)), or introducing mandatory KPIs or OKRs or performance review goals for code review response times, let’s take a look at where we can chop off time more easily.

Unfortunately, there’s a limited amount of data that a) is public (the good stuff is locked at a vault in FAANG), b) contains the code review related metrics that we’re interested in (nope, the GitHub data model doesn’t keep track of many details), and c) is not a toy project (me and my friend Jeff’s dog Zoe working together on an Erlang compiler doesn’t count).

But not all is lost. Many well-known real OSS projects use Phabricator (FreeBSD, LLVM, Mozilla), and Phabricator data has all the goods. For example, FreeBSD has used Phabricator for almost 10 years. A decade worth of code review data from actual operating system development. It doesn’t get much better.

Of course, to investigate all this, the answer was to write another paper. Where do we waste the code review time (the arXiv copy is here)? Theoretically, each separate code review phase can be optimized. Engineers can be nagged (or nudged) by the bots till they lose all the motivation to do anything and ignore all the notifications. Based on our industry experience and intuition, we identified two phases to investigate: time-to-first-response and the time between when code changes are accepted and finally merged. Those periods are indicated as non-productive times in the image below.

First, look at what happens after the code changes are accepted. Apparently, a number of them just sit there waiting for someone to press the “Commit,” “Submit,” or “Land” button. For half of the code reviews, there is no associated post-accept activity, and changes are merged as is.

Depending on a Phabricator project, between 29% and 63% of the overall code review lifetime is spent waiting for the accepted changes to be merged.

What about getting that first response? What would make someone immediately accept your code changes without going through the dance of critique, passive-aggressive critique of the critique, and eventual resentment? Immediate acceptance of the first code review version doesn’t necessarily mean it’ll take less time. However, it means that there’s less frustration involved in the process.

The decision tree indicates that smaller code changes (< 18 SLOC of new code) have a higher chance of immediately being accepted. For changes larger than that, the author’s rank (≥ 46 previously completed code reviews) becomes a factor.

So, what should one do? Well, first the easy fix — either change your instance of Phabricator to start the integration process immediately once the changes are accepted or act like The Gerrit People do: once the change has been accepted (e.g., in Gerrit, by default, you need at least one “Code-Review+2” response) then try to commit and merge the changes ASAP.

Establishing your author rank, aka street credibility, is utterly another matter. There’s no easy way — you just need to take the time and build up your reputation as a committer. Perception is a reality.

Do details matter?

I tend to shake my head whenever a paper says that “code reviews at company X take Y hours or days” or “median code review size is N.” Company X probably has tens or hundreds of projects under development. Everything could be different between the projects: programming languages, the experience of engineers, levels of abstraction, urgency, reliability requirements, etc.

As Jack Reacher says, “In an investigation, details matter.”

Reviewing those five lines of code that change the locking order in the I/O manager in the kernel may take days or weeks of discussion, proof, and validation. The similar five lines in the random tool will get stamped by other engineers in 5 minutes. Both projects are being worked on at company X. The results that yet another code review-related paper will claim will depend on the project that the SE researchers will (randomly?) pick.

We find the differences even in the context of the same OS. After mining 797,879 commits from four BSD family operating systems: DragonFlyBSD, FreeBSD, NetBSD, and OpenBSD, we learn that: a) kernel commits are larger than non-kernel commits, and b) code reviews for kernel code take longer than non-kernel code.

If we had sliced the kernel into drivers, I/O, memory management, file system, etc., have we seen the differences between those ecosystems as well?

Be precise. Don’t generalize.

What’s next?

Well, we need your help. I and almost every engineer I know live in the bubble of FAANG, MANGA, MAGMA, or WTF it’s called right now. It just turned out this way. For all I know, the rest of the world operates totally differently. So, more data is needed. For the the #Science, of course.

If you write or review code for a living, please take our survey!

It only takes 5–7 minutes, and your input will help to change the lives of engineers for the better. With enough responses, we can hopefully understand how reality is shaped and ensure that engineers won’t have to spend their time on nonsensical processes or follow archaic dogmas.

About the author

Gunnar Kudrjavets was a software engineer and shifted bits at Microsoft Corporation from 2000 to 2015. Between 2015 and 2020, he continued his quest to develop reliable and performant systems software at Meta, Inc. (a company formerly known as Facebook, Inc.). His current focus is on wrapping up his Ph.D. in computer science at the University of Groningen in the Netherlands and writing contrarian research papers.