Why fast reviews happen

Back in April, Jim Blandy and I started hacking on a little project in a user repo.

Last Wednesday, without warning, I posted the results of three months of work for review in bug 672829. The whole patch is 336KB (actually 534KB, if you count tests), so I split it up into sections and requested code reviews from eight different JS hackers.

The first review came in before I was even done posting all the patches.

That afternoon, I sent a brief e-mail to the JS team asking for help getting the reviews done quickly. That was the only thing I did that I wouldn’t normally do. By noon Friday, just 48 hours later, twelve of the thirteen reviews had been granted, and Brendan was about halfway through the thirteenth.

The reviewers were:

David Anderson
Andrew Drake
Brendan Eich
Andreas Gal
Blake Kaplan
Bill McCloskey
Luke Wagner
Jeff Walden

Thanks, guys.

The best part: this is no fluke or one-off effort. It’s like this every day in js/src. How did this happen? Why do you get faster code reviews in the JavaScript engine than anywhere else in the project?

I really don’t know, but I have some guesses. Brendan Eich was the module owner for many years, and he always turned reviews around lightning-fast—and not by skimming, either, as you know if you’ve ever read a /be review. (You can read a totally typical one in this bug.) I think Rob Sayre probably had an influence as well. Maybe when you run a team with that “hey, are we all acting like adults here, and if not, why not” attitude for a few years, you get a culture of fast reviews.

Whatever the reason, I’m grateful. Fast reviews make me more effective. Some days, they make my job really exciting.

5 Responses to Why fast reviews happen

  1. In this case, the other smart thing you did is split up the patch into multiple pieces. The review lag on a single 300KB dump would have been _much_ longer…. I wish more people did this.

  2. Sorry, I congratulate on your successful review, but I also have to rant about the fact that some other patches take many months or are apparently never going to get reviewed at all.
    See https://bugzilla.mozilla.org/show_bug.cgi?id=578534
    Is Dan not well? Will someone else take the review? What if the patch no longer applies cleanly? Is there another author that can finish the work? Well, who knows, Sindre was only the second person to fail on trying to fix this bug (see the dupe).

    Again, apologies.

  3. Boris: I’d just immediately ask if something could be split up, when you get a review request for something > whatever size.

    Maybe we should tweak bugzilla to give that warning if you attach a large patch?

  4. @the_dees: I re-requested review from someone else. Dan Witte is doing a startup, last I heard.

    People sometimes fail to do reviews promptly. Ranting (especially about a failure, which is inevitable given humans in the loop) as if it somehow invalidates Jason’s analysis of why he got fast review is pretty poor.

    If there is a systematic problem, please state it squarely. I.e. would we do better to have r? with no name set, and a cadre of reviewers charged to go process the entire queue of such requests? I doubt it.

    If personA has gone AWOL on a r?personA request, try pinging in the bug. If that fails, mail that person, the relevant module owner, and cc: me. That will get results pretty quickly.

    /be

  5. @Brendan Eich:

    I apologize that my rant got a little out of control. I didn’t intend to get personal. It’s not about people. It’s about a feeling of unconsciousness when you’re not completely into the community.
    In fact, I have tried to email several people over the past years since I first noticed this issue and wanted to file it. Some people answered but nothing happened. Then I tried to make people aware of the issue by blogging/writing comments. I got responses, yes, but nothing happend either.

    I’m not blaming anyone. I couldn’t reach Dan, but noticed he apparently still does reviews. So I wondered why there is no definite answer.

    I didn’t know how to make people aware of a systematic issue, thanks for telling me. I’ll try it in the future.

    Thank you for your support. I hope it’ll yield some fruit.

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>