Code review comment for lp://staging/~gary/launchpad/bug724025
Revision history for this message
| Gary Poster (gary) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
2eb648a
demo site
(Get the code!)
On Aug 31, 2011, at 2:13 AM, Jeroen T. Vermeulen wrote:
> Review: Approve
> Hi Gary,
Hi Jeroen. Thank you for the thoughtful review.
> Nice cover letter. Worth a chuckle.
:-)
> The distinction between None and [] makes perfect sense to me. Just a very few nitpicks:
>
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
...
> The comment about dev cache sizes makes me nervous. Is it that noticeable?
It is noticeable if you are testing large values of objects in a page. I was doing this in order to diagnose the problem I addressed here and related ones. The large values both approximated the levels of the problem case and caused the problem sources to be easier to spot.
The way I noticed the cache size is not with speed, but with unexpected SQL queries for values that should have already been cached. Because the cache was so small on devel, values would be evicted in my experiments before they would on production, and so "populate the cache" optimizations would not behave as expected even though they were in fact correct.
> On dev systems I would expect cold-start overhead to dominate anyway. Or if it's not a dramatic issue, I'd just leave that note out.
I omitted it. Ehn. :-)
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
...
> That is a lot of code. Is there no way to ship some of it out to helpers so that it's easier to get the gist of what goes on?
I simplified it a bit. I made a local helper; I think the situation I'm trying to duplicate is not quite right for the factory.
> Also, does it make sense to include the makePackageBranch calls in the query count?
No. That's test set up for the comparison case.
If your question was rhetorical because you thought I *was* including makePackageBranch in the query count, it does not. The QueryCollector gets the queries from the most recent request, so the browser call I made is what we see in the results.
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
...
> There must be a clearer way to say this!
>
> For starters, the two sides of the inner if's "or" could be broken out into separate "ifs." Also, either of the inner if's conditions implies the outer if's condition. So you're free to move those two shortcuts out of the inner if. That also gives you room to explain what each condition means:
>
> if for_branches is not None and not for_branches:
> # Empty branches list.
> return EmptyResultSet()
>
> if merged_revnos is not None and not merged_revnos:
> # Empty revnos list.
> return EmptyResultSet()
>
> if (self._
I was fascinated by my reaction to this comment. :-) I really did not want to do what you said because I did not like the extra comparisons in the common case. That's really a micro-optimization of a non-inner-loop, of course, and I should not be hung up on it, but I was.
That said, I beat myself into submission and did as you described above.
> Or for bonus points, extract the "is this an empty sequence?" into a meaningfully-named function. That way you won't even need comments.
This suggestion appeals to me quite a bit, but amusingly I did not follow it. :-P I think I wasn't sure how generic the functionality would be, where it would live, and so on, and simply following the first path was easier.
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> I assume there's also a test where you let both for_branches and merged_revnos default to None and get a nonempty list back?
Yes, that existed before I came along.
Thank you again!
Gary