mgedmin / objgraph

Visually explore Python object graphs
http://mg.pov.lt/objgraph/
MIT License
753 stars 72 forks source link

Add a growth() function that doesn't print #8

Closed jmeickle closed 7 years ago

jmeickle commented 9 years ago

I've been using objgraph in my Python roguelike. Peak object count is interesting info, but show_growth() prints, which doesn't work when you're using curses to control the console. I added a growth() function which returns the data used in the table.

Not strictly related, but there's a second commit in here that lets you return the dot file as a string without writing it to disk or opening it, which lets me have more control over the results (like postprocessing, or piping the output to something).

I didn't have time to make further changes, but while working on the code I came up with a few suggestions:

  1. I wouldn't recommend printing inside of a library, or at least, you should provide a toggle to override it. There are a lot of circumstances where print either won't work or will interfere with what the program's supposed to do. A better approach is for the functions to accept a printer argument (falling back to print), and then call printer() in functions instead of print() directly.
  2. There are a bunch of places where the library isn't as flexible as it could be. For example, show_growth() could have an option for current as well as peak object count/delta. Or in show_graph(), functions like obj_label could be arguments that can be overridden. (An object-oriented equivalent that allows subclassing the render functions would be even better.)
  3. Storing the data from show_growth() in function defaults isn't a great idea. How they work is poorly understood, and I suspect it'll lead to someone trying to use it as a 'reset', which doesn't work:
objgraph.show_growth():
wrapper_descriptor             1043     +1043
function                       1004     +1004
builtin_function_or_method      660      +660
method_descriptor               539      +539
dict                            438      +438
weakref                         324      +324
tuple                           197      +197
member_descriptor               188      +188
list                            168      +168
getset_descriptor               155      +155

objgraph.show_growth():

Create dictionaries:

objgraph.show_growth(peak_stats={}):
wrapper_descriptor             1043     +1043
function                       1004     +1004
builtin_function_or_method      660      +660
method_descriptor               539      +539
dict                            439      +439
weakref                         324      +324
tuple                           197      +197
member_descriptor               188      +188
list                            168      +168
getset_descriptor               155      +155

objgraph.show_growth(peak_stats={}):
wrapper_descriptor             1043     +1043
function                       1004     +1004
builtin_function_or_method      660      +660
method_descriptor               539      +539
dict                            439      +439
weakref                         324      +324
tuple                           197      +197
member_descriptor               188      +188
list                            168      +168
getset_descriptor               155      +155

objgraph.show_growth():
dict      439        +1

A better option would be to store stats at the module level and add reset=False to relevant functions.

Suggestions aside - thanks for all the great work on this! It's a really wonderful tool :) I wish I'd tried it sooner...

mgedmin commented 9 years ago

The two unrelated features should be split into separate pull requests.

The new code in show_graph() doesn't actually work (see the test failures in Travis).

Other than that, I'm in favour of both features. And yes, a redesign of the API to be more flexible would be nice to have too.

Can you rebase this PR to only contain one feature (the addition of growth), make it work (fix tests), and make the docstring user-friendly (which will involve a large amount of duplication in argument names, I'm afraid)?