Launchpad itself

Merge lp://staging/~gary/launchpad/bug821531 into lp://staging/launchpad

bug821531
Merge into devel

Proposed by Gary Poster on 2011-08-08

Status:

Merged

Approved by:

Gary Poster on 2011-08-08

Approved revision:

no longer in the source branch.

Merged at revision:

13632

Proposed branch:

lp://staging/~gary/launchpad/bug821531

Merge into:

lp://staging/launchpad

Diff against target:

808 lines (+387/-131)

4 files modified

.bzrignore (+1/-0)
lib/lp/services/profile/profile.pt (+45/-21)
lib/lp/services/profile/profile.py (+108/-8)
lib/lp/services/profile/tests.py (+233/-102)

To merge this branch:

bzr merge lp://staging/~gary/launchpad/bug821531

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Benji York (community)	code	2011-08-08	Approve on 2011-08-08
Review via email: mp+70738@code.staging.launchpad.net

Commit message

[r=benji][bug=821531] Make it possible to request a pstats ++profile++ with "++profile++pstats" (in addition to a callgrind one, still available as "++pstats++callgrind").

Description of the change

This change adds the ability to get a stdlib-friendly profile output to ++profile++. This lets you dig in farther than KCacheGrind does, which has a maximum of the top 499 calls displayed for a given sorting.

Lint is happy except for being confused with the start of profile.pt.

Thanks

Revision history for this message

Benji York (benji) wrote on 2011-08-08:

This branch looks good. I can't wait to try it out. Here are some
relatively minor comments.

The name "pstats" might be a little better than "stdlib" because it's a
pstats-formatted file that's generated. For that matter, now that there
are more than one output formats, "log" might be better replaced with a
word that describes the generated format ("callgrind") while perhaps
leaving "log" as a backward-compatible alias.

Line 111 of the diff: the docstring summary for StdLibStats wraps.

Line 130 of the diff: JavaScript has warped your brain, you're missing a
comma on the last element of "mapping".

Line 183 of the diff, teeny-tiny suggestion: you could use '-'.join(...)
instead of '%s-%s-%s-%s' % (...).

Lines 235 and 256 of the diff: more docstring wrapping. Maybe those
should just be comments instead.

review: Approve (code)

Revision history for this message

Gary Poster (gary) wrote on 2011-08-08:

I made all changes that you suggested/requested except the '-'.join(...) approach: I prefer the template approach for the filename.

I changed all test docstrings to comments: many of them were wrapping, and as you hinted, comments are reasonable and maybe even preferred for these descriptions anyway.

I'm asking you for a re-review given the size of the changes.

Lint is still as happy as it was ("happy except for being confused with the start of profile.pt"). Thanks!

Revision history for this message

Benji York (benji) wrote on 2011-08-08:

This looks good. Thanks for considering my input.

One thing I didn't notice the first time around (assuming I'm reading
the code correctly): starting on line 279 of the diff, if the user
provides just "log" they will get callgrind output, if they provide
"callgrind" and "pstats" they get both, but if they provide "log" and
"pstats" they will get just "pstats". Given the internal nature of the
code, this corner case is not pressing.

review: Approve (code)

Revision history for this message

Gary Poster (gary) wrote on 2011-08-08:

Thanks again. I added a test for the "log,pstats" behavior: it is intentional ("log" is ambiguous and "pstats" is explicit). Note that if you request "callgrind,pstats" you actually get only callgrind and a warning that we can't do both at the same time.