3 ms·

I sometimes fall into a similar trap. Usually on the larger changelists where I was struggling to keep the focus necessary to find the serious bugs, but not be

I sometimes fall into a similar trap. Usually on the larger changelists where I was struggling to keep the focus necessary to find the serious bugs, but not being comfortable only looking at half the files in the code review - feeling like I'd not be doing my part of the job I didn't at least look at them. I have to remind myself that there's a limit to how useful nitpicking can be, and that it can become counterproductive as well. Sometimes the quite serious bugs are just hard to notice, as well. I recently reviewed a large changelist dealing with lots of multithreading and locks, and cases where locks aren't taken to avoid deadlocks. I have 0 confidence I caught all edge cases, which terrifies me for multi-threaded code.

sauceop 8y ago
I don't have your context, but if the changelist author is writing multithreaded code where there's no way that a reader can convince themselves of the correctness, then my heuristic is that the burden is on the author to improve the code to be easier to reason about and add enough tests to exercise all of the interesting code paths. There are always techniques to simplify code or make it easier to grasp.
- MaulingMonkey 8y ago
  I have similar heuristics. I'm drawn to Rust because I really like the idea of having to prove it's at least data race free to the compiler (not that this will solve the problem of deadlocks.) In most situations I'd push back on code that got half as tricky with it's multithreading. Simplifying is easy. Early versions of this code, years ago, were simple. They weren't even multithreaded! Just a simple implementation to unblock other devs. But now it's a highly used bottleneck that must be high throughput, low latency, support asynchronous cancellation of requests, interacts with the main thread for third party APIs that aren't thread safe (such as d3d9), interacts with the main thread for our own APIs which aren't thread safe by design (our debug replay system replays the events of the main thread), but must avoid synchronizing with the main thread for performance elsewhere... Simplifying this without performance or feature regressions is significantly more difficult. Possible, but difficult. And benefits from slow, testable, incremental changes. But that means reviewing incremental diffs on the large and complicated existing system in the interim. At least it has some of our best test coverage, including lots of tests to help try and tease out threading bugs. They don't always succeed at that, but they do help.

sauceop 8y ago

I don't have your context, but if the changelist author is writing multithreaded code where there's no way that a reader can convince themselves of the correctness, then my heuristic is that the burden is on the author to improve the code to be easier to reason about and add enough tests to exercise all of the interesting code paths. There are always techniques to simplify code or make it easier to grasp.

MaulingMonkey 8y ago
I have similar heuristics. I'm drawn to Rust because I really like the idea of having to prove it's at least data race free to the compiler (not that this will solve the problem of deadlocks.) In most situations I'd push back on code that got half as tricky with it's multithreading. Simplifying is easy. Early versions of this code, years ago, were simple. They weren't even multithreaded! Just a simple implementation to unblock other devs. But now it's a highly used bottleneck that must be high throughput, low latency, support asynchronous cancellation of requests, interacts with the main thread for third party APIs that aren't thread safe (such as d3d9), interacts with the main thread for our own APIs which aren't thread safe by design (our debug replay system replays the events of the main thread), but must avoid synchronizing with the main thread for performance elsewhere... Simplifying this without performance or feature regressions is significantly more difficult. Possible, but difficult. And benefits from slow, testable, incremental changes. But that means reviewing incremental diffs on the large and complicated existing system in the interim. At least it has some of our best test coverage, including lots of tests to help try and tease out threading bugs. They don't always succeed at that, but they do help.