Code review comment for lp:~mir-team/mir/enable-dynamic-buffer-queue

(1) Bug 1317403 was really a consequence of bypass needing triple buffers, so read on... Also if you find a bug invalid, the first place it should be mentioned is in the bug.

(2) That's a bit surprising. For most of the year or so since I implemented bypass, 3 buffers was the minimum. Less than that and the client/server would be waiting for each other indefinitely (hence visibly freeze). And a month ago I could still reproduce some deadlocks, sometimes. But I can't today. It looks like possibly:
  (a) Alan's newish async client buffer swapping solved the primary problem, as there is now significant time during swap_buffers where the client owns zero buffers. So that leaves the minimum 2 available for the server to bypass with, and then release one back to the client. Also...
  (b) Bug 1319765 was unsolved around the time I was seeing deadlocks most recently so that could have been all of the remaining deadlocks, now solved.

Although I'm not yet 100% confident, it does look like bypass can now work with only two buffers. A win for performance!

(3) It's cheating because I do remember I could only reproduce the original performance bug at 16ms. It was right on the edge but I guess too brittle to rely on such timing. But I can't see either bug 1241371 or bug 1241369 occurring any more so I guess that is the important thing.

(4) BufferAllocBehavior: I think is poorly named/unnecessary. A more straightforward design is to simply provide "min_buffers" and "max_buffers" per my original branch. No enum required.

(5) BufferAllocationPolicy: There is precedent but I maintain that anything-"Policy" is a poor choice of class design. Policy is not as concrete as other concepts to name classes after, which would be easier to understand. "Policy" is an indication that the class design needs rearrangement. Both BufferAllocBehavior and BufferAllocationPolicy can be replaced by three ints; min_buffers, max_buffers and shrink_threshold. One of those ints already exist so it's obvious that replacing an enum and class with two ints is a more straightforward design.

This is just a high level review. I haven't started to read the code really but it mostly looks like my double branch.