-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New method to get profiler data as python object #13
base: master
Are you sure you want to change the base?
Conversation
pprofile.py
Outdated
@@ -883,14 +962,37 @@ def runpath(path, argv, filename=None, threads=True, verbose=False): | |||
""" | |||
_run(threads, verbose, 'runpath', filename, path, argv) | |||
|
|||
_allsep = os.sep + (os.altsep or '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantage of defining on module is to compute once on module import rather than once per call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, what you say is true, in the general case. In the specific case here however - given that this code is executed at most once per execution of pprofile (correct me if I'm wrong) - then shouldn't readability/style take a higher precedence?
Basically unless a profiler tells me something is significantly faster in the less good-looking style, I opt for the good-looking style :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tastes... :)
Another aspect is that this changes a piece of code which does not have to change for the new feature. Like the s/close/close_fn/ change in main
. This reduces (admittedly by a trivial amount here) patch (and later, VCS history) readability. I'm not opposed to style patches (although they can quickly degenerate in style wars), but I do not like patches which touch both functionality and style outside of that functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I can't argue with that; 👍
I commented on minor, main-feature-independent issues in the code. On the main feature, I am a bit worried about the impact on forward compatibility: I wonder how dicts of lists of dicts (etc) work as an API. For example,
The 3rd item can take 3 types depending on some internal details. Caller code will look like a 3-branch "if", and if it does not it is likely to fail when profiling another source code. Is caller supposed to treat this as a black box ? How to document this ? How to prevent naïve introspection of this value ? From a quick look at the |
On Other than the So my goal here was to make Does that sounds like a bad idea? I would be nice for the user not to have to subclass this, but simply be able to call I'm open to changing the actual format of the object returned by What are your thoughts? |
I would tend to do this using extra run-time introspection, like what I did in
On the avoiding-code-rot level, it is a good idea. But as I said, I think it would be better to expose the interactive-usage-friendly layer directly rather than an extra level of internal state: it is easy to defend breaking (by future evolutions - and I prefer not to presume of what evolutions will do to the internal data structures) the API in the former because caller is a human on a command line: he can quickly see something changed and adapt. But it is hard to defend breaking the API in the latter case because python structures are just too tempting to use in another program (integrating pprofile), and once caller is a program nothing auto-corrects anymore. So I would rather:
|
Quoting `vpelletier': > Well, in the case of qcachegrind, this is specifically for the > callgrind output, so it made sense to be local to that method.
Will reserve style-changes for another branch to eliminate unnecessary noise in this branch's diff: - Reverted `allsep' - Reverting `close' -> `close_fn' (style-change) - Reverted `x' -> `path' (style-change)
The `relative_path' kwarg is now mandatory, so user has to choose (since no default makes sense or is otherwise intuitive). Also updated the docstring.
Quoting `vpelletier': > This test will prevent any output in statistic mode, as no time is > tracked but only hits. I think it can be dropped and contained block > be un-indented without any other change.
d26119a
to
79f7d2c
Compare
Hey!
I needed a way to access the profiler data programmatically; to do this I've implemented the
get_stats
method, and any other changes there are to reduce code-duplication.The next steps I'd like to work on would be:
annotate' to use
get_stats`callgrind' data to the object returned by
get_stats, and rework
callgrindmethod to also use
get_stats`To see how this new method is used, please check out the
pydata' branch. In iPython, you can basically perform things like
ls,
cat, and
grepthe files that the profiler picked up, there's a lot of room for awesome features, but I've only been working on what I need most. Here's a quick demo using a sample file called
sample.py`:And now, the profiler: