Archive for the ‘process’ Category
On Build Integrity
One of the most important things any studio does is build the game. Compile tools and executables, preprocess resources, pack levels and so on. “Build integrity” is a term I use to cover a set of standards and processes intended to make sure the game build is consistent and reproducible. By this I mean that:
- Build output is independent of environment.
We get an identical build regardless of the machine, its environment, and the person doing the build. In “its environment” I include things like OS patch level, installed software and configuration, environment variables, and registry settings. - Build output is independent of time.
We can reproduce, exactly and simply, builds in any platform and configuration using source materials from any point in time. No special prep must be done to a system, such as downgrading installed software to run an old build. - Build output can be traced to its source.
We can reproduce any build given just a few pieces of information about precisely how it was built, which we can encode compactly into the executable and archive folder name. This lets us match up issues filed against a particular build with exactly what versions of source materials went into that build.
Most experienced studios are already doing this, but not all! At Oberon we worked with a lot of other studios and sometimes when we needed to build their game ourselves we’d hear great stuff like “we couldn’t find all the source code”, “this is an old version but it’s probably fine”, and “Bob’s machine did the builds but he got fired and nobody can find his machine”.
I know of one big pro studio that, upon shipping, takes their build server and sticks it in a closet in case they ever need to build the game again. Is that a safety measure? I’d rather trust a solid build process that is regularly in use on well-managed servers than some old machine gathering dust in the closet. Builds frequently end up rising from the dead! Someone always wants to port it to another platform, reissue it in a ‘classics’ with bugfixes, add integration with social networks, or pack it into an OEM build with new languages added. Or even send out a patch years later for paranoid things like Y2K compliance (remember that?).
Backup servers on ice or restored from an image taken at ship time may work, but we can do better.
Why All the Bother?
You could ask why we should bother with defining a lot of process and standards for the game build. Not that it’s a lot of work, though it could be a major change in mindset if you’re used to just having Bob copy builds up from his machine.
Build integrity is important because we practice science in diagnosing and resolving issues in the game. Detached observation, hypotheses, evidence, elimination of variables, reproducibility, all of that. On modern games on platforms with many processors running simultaneously, and a lot of nondeterministic input from multiple players and the network, we can’t afford to mess around with guesses. We must be able to trust the evidence that we base our assumptions on when we start investigating.
The alternative is madness. Seriously, madness. You don’t want to be near someone who has been debugging some insane problem for two days only to find out that they’ve been working with the wrong build. Whether it’s from a bad build number put into the bug report, or the build having been done by someone with hacked local changes, or consistency problems in the depot, this can really turn a normally nice, mellow person into a rage machine. After hours of carefully eliminating all possible variables and laboriously stepping through optimized assembly, having to start completely over is unbelievably frustrating. Most bugs obviously aren’t like this, but the ones that are always seem to show up at the end of a project and threaten ship dates and marriages.
And that’s just the start. There are a lot more things that can suffer from a lack of build integrity, which I’ll be covering along the way in the next post. The bottom line is that without evidence we can trust, we can end up wasting a lot of time in engineering and testing. We want to set up our build process and practices in a way that ensures high build integrity. The build is the most important output of the studio (besides paychecks of course) and it must be protected.
This goes beyond simply archiving the executable and debug images, or having a dedicated build server. It’s a way of life. Luckily, it’s also pretty easy to set up, and has lots of side benefits such as accelerating setting up a new team member or project. The build platform includes the development platform. More on that in the next post.
Peer Code Reviews: Good Commenting Practices
This is the fourth in a series of posts on our peer code review process at Loose Cannon.
Somewhere in the middle of the third post, I started to talk about the “make comments” part of the process, but it’s a big subject, deserving its own entire post. So here we go.
Comments in a review are where the real goods are delivered. This is where we get all the benefits that I had talked about way back in the first post.
What We Don’t Expect From Reviewers
First let’s talk about what reviewers aren’t expected to do when they’re reviewing a change.
Reviewers aren’t expected to catch everything.
It’s impractical and arguably a waste of time. Knowledge-spreading, mentoring, and so on are more of a “seeping” process than a hard core lesson plan. The idea is that, eventually, with enough reviews and shuffling of reviewers, the knowledge will spread throughout the entire team. There’s just no need to focus on catching every single thing in every single review.
Reviewers aren’t expected to catch deep or systemic design problems.
A changelist is a snapshot of a small part of the game. It’s really hard to try to see the big picture through a pinhole. Reviewers will often open up their editor and browse around in code outside of the change during a review, to get more context. FishEye’s browsers and search (and blame!), and Perforce’s time lapse view help here a lot too. But this only goes so far.
At some point, you’ve got to throw up your hands and say “we’ve got to talk, I can’t see what’s going on here”, head to the whiteboard, and discuss it in person.
The reviewer is not (necessarily) the boss.
Reviewers do not necessarily have the authority to enforce changes, nor should they have extra responsibility for the quality of code they didn’t write. The reviewee maintains responsibility for their own changes.
We are tapping into reviewers’ brains and schedules to help make the entire project better. This is a service they provide, not an opportunity for dictatorship. While it is a requirement of our process that all reviewers’ comments must be resolved, this does not necessarily mean “do what the reviewer says”. Ultimate responsibility and authority remains with the reviewee’s lead.
Now, it’s pretty easy to end up in dictator-speak mode when you’re in the zone, ripping through reviews, making comments. It helps to soften more subjective comments with phrases like “I suggest”, “are you sure this is the best way?”, and “this is totally optional and my opinion, but…”.
What Reviewers Seek
Ok, now on to what reviewers are actually commenting on. Reviewers are looking for the following kinds of things, in no particular order of priority.
Are There Architectural and Domain-Specific Issues?
Every project has experts in different problem domains. You want these people reviewing changes in areas in which they have expertise. Graphics, scripting, debugging, architecture, assembly, tuning, you name it.
Here are some examples I picked out at random from our reviews.
All we’re doing here is looking for “gotcha’s”: hidden rules in systems that you know well are especially important. Or perhaps better, more efficient ways to do things. Or how new code should fit into old code and interoperate with other systems.
This isn’t just for immediate course correction. With each review comment in a specific problem domain, the reviewee learns how to do things better in the future, so we don’t hit this again. And, often, the reviewee will run through their other code where the same mistake was made (but not caught yet) and fix it.
This is a wonderful way to spread knowledge while improving the code in the immediate changelist.
Are They Following Best Practices?
Here we are drawing heavily on the unique career experience of the reviewers, who are looking for things like:
|
   |
A lot of this can really subjective and often results in spirited debate. But this is a good thing – everybody learns something! And often we’ll agree to disagree and move on. However, if the same issue comes up again and again, then we can add the lead to the review and ask them to make a call to resolve it.
| To the right is a sample clipping with a best practices discussion that will lead into an offline meeting. |  |
Are There Any Opportunities to Mentor?
Teams are often made up of people with a wide range of experience levels. Review comments can be a great place to mentor a more junior engineer. If they’re sharp and fearless, they’ll challenge you on comments they don’t agree with or understand. Instead of getting mad and replying with a “just do it, this is the best way” – take advantage of the opportunity!
If you instead take the time to really give them a good explanation of why you made the comment, a couple things may happen. First, they will learn something. Great. But another possibility (this happens to me a lot) is that by forcing yourself to explain why it must be done that way, you find out that you actually don’t have a good reason. Maybe your reasoning is based on religion, or outdated techniques, or wasn’t completely thought-out. I like when this happens because it makes me a better engineer. Make sure the team knows that as a reviewer you expect to be challenged.
Comments in Crucible (our code review tool of choice) have permalinks as well, so it’s easy enough to link to the discussion from the team’s wiki for spreading the word.
This has been one of the more successful parts of our code review process. People will ask questions and say things in text form that would never happen in person. It just doesn’t come up as often in casual conversation to talk about a lot of seemingly minor things like why a particular naming convention exists. In text, when you’re directly reviewing code, it’s a natural part of the process to say “why does it need to be this way?” and can easily be done in a non-confrontational manner.
Are They Adhering to Our Coding Standards?
Not long after we started the new code review process at Loose Cannon, we sat down to hammer out an initial set of coding standards. We were planning on having coding standards anyway, but it became clear right away when we started reviews again that we needed standards immediately. It’s very hard to review code where every person has their own weird style and habits.
Comments about coding standards are simple and easy, and should be short. We have a Confluence doc with our standards, so as you notice things that don’t adhere, flag them. This is a good way to teach new engineers our existing standards, as well as updating everyone on the occasional new standard.
On our current game, we backed off from most of this when we were very close to shipping. At that point you’re writing a lot of junk (particularly certification compliance) just to get the game done that you don’t intend to carry forward to future games. More nitpicky stuff like coding standards is just not a priority in the final weeks.
Did They Write A Good Changelist Description?
This is a relatively new thing we added to reviews. It is really a best practice, but I wanted to call it out as a special item because good changelist descriptions are so important when debugging problems with unfamiliar code. And so many people do it wrong. [I need to write up a post on how to write good changelist notes. People always make the mistake of commenting on the ‘what’ they changed, and missing the ‘why’. Any fool can do a diff and find out what changed, but six months later remembering why it was changed? That needs good changelist notes.]
When our crucreate tool tells Crucible to create a review, it automatically sets the Objectives of the review to the contents of the Perforce changelist description. This is important for a couple reasons.
First, obviously, it helps guide the reviewers by answering questions in advance about the point of the change. Reviewers should always check the objectives to get background on what they’re about to review. Saves a lot of time asking questions in comments that are already answered in the objectives.
And second, as it is a part of the review, it can and should be commented on (with a general review comment). No more lazy, useless changelist descriptions!
It’s for this reason that we modified our Crucible installation to pre-expand the objectives (normally collapsed by default) any time a review is viewed. Otherwise people never saw the objectives because they never bothered to expand them. [Thanks to the nice Atlassian support folks for their help here.]
The “Final Comment”
The last comment a reviewer makes, as documented in the last post, is to say what should be done with the review. Here are some samples I picked at random.
| The most common final comment is something like “looks good, check in”. |   |
| This one is a little more complicated. Obviously some in-person discussion has happened in between, as well as an incremental UPDATE 1 that was attached. |  |
| Sometimes the last comment is the first comment as well. I found a great example of sharing knowledge using reviews. Here, a reviewer got added specifically so they could learn about JSFL in Flash. |  |
And that’s a wrap! Just in time too – I’m getting on a plane in a couple hours (the first of three) to leave Quito and head on over to Sydney.
Series
My full series on code reviews:
- Part 1: Peer Code Reviews At Loose Cannon
- Part 2: Peer Code Reviews: First Attempts
- Part 3: Peer Code Reviews: Success!
- Part 4: Peer Code Reviews: Good Commenting Practices
- Part 5: About Our Crucible-Perforce Bridge
- Part 6: Peer Code Reviews: How Did We Do?
Peer Code Reviews: Success!
This is the third in a series of posts on our peer code review process at Loose Cannon. In the first, I talked about what code reviews are and why we do them. The second documented our initial attempts at implementing a code review process.
In this post I’ll cover how our final process turned out. We’ve been doing this for perhaps the last year or so, through many milestones and the final product release, so it has been battle-tested!
Of course, as with all processes, what works depends on the team and culture already in place. It took us a while to settle on this one and I’m sure it would need adjusting at other studios.
Third Attempt: Crucible + Tool + Process
I kept watching Atlassian for updates, and it wasn’t long before they released a version of Crucible that supported patch-based reviews (I think it was 1.1 or 1.2). Yay!
Support for patches wasn’t great; changes coming from patches were not first class citizens like reviews created from a submitted changelist were (note that this is something they have been improving in recent versions). But it was good enough. You could take a patch file of local changes, upload it, and select it in as part of a new review. Awesome.
New Process Requirements
With everything we needed from a collaborative peer review tool in place, a few of the seniors on the team met and designed a new process for our rebooted code reviews. We had the following requirements:
- Every change to game or engine C++ code must be reviewed.
Tools and game scripts were not included in this process yet. We wanted to get started slowly and narrowing the scope of what got reviewed, and expanding it later on. - Code reviews must precede checkins.
We would continue the previous process’s requirement of review-before-checkin, for the reasons stated earlier. I ran periodic queries at first to make sure of this until everyone got in the rhythm of the new process. People got it pretty quickly. - Reviews must include a “primary reviewer”.
There would be a core group of three primary reviewers (made up of Matt, Andy, and me to start), and one of us had to be part of every review. We would expand this group over time at Matt’s discretion.
Initial Concerns
We were at first a little worried about the review load among the three of us. I did a query of Perforce to get a feel for how frequent and large the checkins tended to be. We estimated that 20% of our time would go into reviewing code, and had to think hard about whether or not we felt the benefits were worth this cost. We also decided to bring new people into the core group as quickly as possible to help with the load, perhaps within a few weeks.
As it turns out, we didn’t have any trouble handling the load, which was far lower than we had expected. A review or two a day was probably what we each averaged, maybe 5-30 minutes total, depending on complexity. So we ended up keeping the 3-person core group for a while. We pretty much forgot about adding more people until a couple other members of the team asked to be included and we expanded the group.
Another concern was that the process of creating patches and uploading them by hand would be a pain in the ass and error prone. The technical part of the review process had to be really simple or it would be harder to get people on board with the whole thing. Clearly a tool was needed. Luckily, Crucible has a SOAP interface! Well, it had a SOAP interface. They recently switched to REST, so I lost my convenient WSDL and now have to maintain my .NET wrapper of their API by hand (ah well). But whatever.
To solve this, I spent a couple days adding a new crucreate command to my p4x tool to automate creation of reviews from pending changelists in Perforce. This tool has gone through many revisions since, and I will do a full post on what we’ve got now and how it works in a future post.
The New Review Process
Given our requirements, and the new tool, we created the following three-stage process for peer code reviews. All documented in Confluence of course!
Stage 1: Create Review
This part is pretty quick to do, rarely more than a few minutes and usually under 30 seconds:
- Prep a pending changelist as if you were about to check in: isolate it to the minimum required for the change, give it a good changelist description, build all platforms and configurations, etc.
- Right-click the changelist in P4Win/V and select “Create Crucible review from changelist…”. This runs p4x crucreate, which puts up a dialog box with available reviewers listed. You can also do this from a command line.
- A primary reviewer is pre-selected at random but you can change it (*) if you want someone specific.
- Select secondary reviewers (*), using your judgment. Who is the de facto owner of the code? Who else is affected by this change? Who has domain expertise that would be useful? Who will be upset if they see your checkin and weren’t part of the review?
- Hit OK! A review is automatically created by p4x and comes up in the web browser in the Draft state, reviewers set up, patch uploaded, and so on.
- Make any necessary comments on your own review (**). This is also a good time to review the diffs and look for bonehead mistakes (like temp/hack/test code you didn’t intend to check in).
- Consider abandoning the review and going back to the code for another pass. (***)
- Hit the Start Review button! All the reviewers will get notified by Crucible via email that they have a job to do…
Some notes:
(*) In choosing reviewers, it’s a good idea to instant-message people first to ask if they’re available to do a review, especially if you’re really itching for feedback or you want to check in ASAP. You never know what schedule other folks are on, mentally or physically! I usually turn around reviews fast when I’m looking for a distraction, but other times I need to focus and will put off reviews until the next day. It’s always good to check.
(**) For example: noting what your intention is in different areas, calling out chunks of code that need special attention, or asking questions like “this part is a mess, is there a better way to do this?” And if you’ve added people to a review for their domain expertise, you can save their time with a comment like “Joe – I added you just for the graphicsmgr changes – did I do this goofy GL state setting stuff right?”
(***) Well this is a little odd, eh? Perhaps it’s just me, but prepping a review always makes me think about the problem from a different perspective, and I often realize I screwed up something deep, or didn’t solve the problem fully, or forgot to test a few things, and so on. So I’ll abandon the review and head back to the code for another pass. It’s a lot less embarrassing to figure this out when a review is in Draft and not started.
Stage 2: Perform Review
This stage is very fluid and could take minutes to days of time to get through, depending on the type of change involved. It has these main tasks:
- Reviewer Task: Make Comments
Here we go line by line through the diffs and make comments and ask questions. This is where the big win is in doing reviews! Right here, in the middle of this giant post! Unfortunately, what to comment on and how is itself a really big topic, so I have to cover that in a later post. - General Task: Discuss
Questions need answers obviously. And comments that the reviewee doesn’t agree with or understand will need some discussion. Most discussions we’ve had are very short, but some can get large and end up needing in-person discussion or possibly escalation to the team lead for a call to resolve. - Reviewee Task: Address Comments
”Address” does not mean “Do What Reviewer Says”. The reviewer is not the boss. “Address” means that the reviewee needs to do something with every comment a reviewer makes. So that means: making the requested change, answering the question, arguing the point, raising new questions and so on. Note that ignoring the comment is not a valid choice. - Reviewer Task: Mark Complete
Once a reviewer has made all their comments and has had their questions answered to their satisfaction, they (a) mark it complete in Crucible, and (b) add one final comment. It’s a general comment saying if they’re ok with things going forward. For example, “check in after addressing comments”, “send back with an incremental review”, “this whole change needs to be reverted, I’ll come talk with you” (perhaps by a team lead), and so on. - Reviewer Task: Talk In Person
Sometimes it’s just not going to work through Crucible or any tool, and you need to sit down with the reviewee and have them explain how it all works to you. Especially if it’s a huge or weird change that just doesn’t work well in diff form. Afterwards, the reviewer can go back and make informed comments, or maybe the reviewee will be able to bypass the rest of the review entirely if it all came out in the 1-on-1 discussion.
Now, while a change is going through this review process, what is a reviewee to do besides replying to comments and making fixes as they are addressed? They should be working on other things, by using a separate Perforce client, a private branch, or by working on files that do not conflict. Whatever works. They shouldn’t be sitting on their hands waiting for the review to be done so they can check in.
Of course, sometimes you have a high priority fix that needs to go in. Maybe some people are waiting on the edge of their seats for your change. And sometimes people who can’t manage their inbox don’t see or ignore the review notification email. Nagging via instant messaging does a good job of solving this problem.
Stage 3: Close Review
This final stage takes just a few seconds. Finished reviews go to one of several places:
- Submit And Close!
This happens when everybody is satisfied. It is the result of most of our reviews, depending on the stage of the project and scale, risk, impact, etc. of the change. We’ll go here if all the comments result in trivial changes and the reviewers have said they don’t need to see a new review. So check in the code, take the Perforce changelist number, and paste it into the Crucible review (like “checked in as #37132”) when summarizing it, then close. - Add Incremental Review
This happens when reviewers want to review new code changes based on their comments. Crucreate can “diff the diff” and add incremental updates to existing reviews with only what has changed since the comments (more on how this works in a future post). After doing this, notify the reviewers that more info is available and they’ll go back and do another pass on the update. Repeat as necessary. We’ll rarely have more than one or two incrementals tacked onto a review. - Create New Review
A totally new review is required when the changes based on comments are really hard to review incrementally. No big deal – just crucreate a new review, then summarize the old review with the ID of the new one, and we start the process fresh. Now, if a change goes through more than a few separate reviews, we probably need to have a whiteboard discussion to sort it all out before the next review is created. - Abandon!
Head back to the drawing board. This happens when there is just too much work to do to make things right. Create a new review when the code is ready again. Perhaps 5% of our reviews end up this way.
And that’s the step-by-step of how we do reviews at Loose Cannon!
I’ve gone into a lot of detail that may make it all sound pretty laborious, but that’s the level I like to write at. In practice, our reviews tend to go fast and smooth. It really feels like a natural part of our process now.
Coming Up
Well, it turns out I have a lot more to say about our review process than I thought, so I’ve had to continue to break this down into multiple topics. Maybe I should have a list of what I intend to hit in future posts, and hope it won’t break down further:
- How reviewers make comments. What we comment on and why!
- How this whole process worked out for us. Did we solve the problems we set out to solve? What new ones arose? What does the future hold?
- How the crucreate tool works in detail. With action screenshots!
And with our visas expiring soon, Ally and I are nomads again, so it’s been really hard to find time outside of work to write. But I’m on an 18 hour bus ride right now, so I’m going to see if I can queue up some stuff to post when we get to Máncora. We’ll be there for a week, then it’s on to Quito, Sydney, Brisbane, and back home-home in Seattle on August 9th. I’ve heard we’re missing a really nice summer. To be honest, what I’m really missing is Boar’s Head pickles.
Until next time!
Series
My full series on code reviews:
- Part 1: Peer Code Reviews At Loose Cannon
- Part 2: Peer Code Reviews: First Attempts
- Part 3: Peer Code Reviews: Success!
- Part 4: Peer Code Reviews: Good Commenting Practices
- Part 5: About Our Crucible-Perforce Bridge
- Part 6: Peer Code Reviews: How Did We Do?
