Skip to content
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

Refactor graph code #40

Closed
wants to merge 30 commits into from
Closed

Conversation

tonysyu
Copy link

@tonysyu tonysyu commented Nov 9, 2014

Sorry, this is a bit of a code dump, but I tend to refactor code when I'm trying to understand it. I completely understand if this is too massive a change for merging.

Note that PStatRow is renamed to PStatsNode, while PStatGroup and PStatLocation are removed. The latter was unused and the group was only used temporarily to package multiple PStatRows, but only the first is ever used.

Other changes:

  • Add user-specified tree-depth for upload. This is a work around for issue Improve large profile handling #12.
  • Tests! And Travis CI
  • I've added a comment about some weird loop-back behavior in stats_to_tree_dict. Basically, when the size of child calls doesn't add up to the parent size, the parent is linked to itself. I don't understand what this is trying to accomplish, but I didn't want to remove it (since I didn't understand it).

PStatGroup was never used on its own.
The methods defined there were unused
Note that the CLI still doesn't properly initialize the app.
- PStatGroup to PStatsForest
- PStatRow to PStatsNode
There really shouldn't be an `obj.caller` and an `obj.callers`.
`PstatsLoader.forest` points to all known trees.
The profile uploader only used the slowest tree.
@embray
Copy link
Collaborator

embray commented Nov 11, 2014

Thanks for this! I'll have to take a closer look at it later.

IIRC the pstatsloader module came directly out of runsnakerun (with permission) hence parts of it seeming unused/superfluous. As such I'm a little hestitant to go removing/changing things in there unless there's some marked improvement in doing so.

It looks like you also changed optparse to argparse which I wholly support. I'm not sure why it wasn't using argparse (possibly for support of older Python versions at the time).

@jiffyclub
Copy link
Owner

Thanks for having a go at improving snakeviz! Updating snakeviz has finally floated to the top of my to-do list, and there are major changes coming. In particular I'm planning to remove almost all the Python business logic and re-implement it in JavaScript on the client side (e.g. #19, but not exactly reimplementing pstatsloader). So, unfortunately, my plan is to delete almost everything you've changed here. 😬

But, I don't have my update working yet, so if we wanted to use some of this in the meantime I would be okay with that.

BTW @tonysyu, the reason we add add a child linked back to the parent is to represent the internal time in a function, e.g. time not spent inside other functions. I seem to remember that the visualization came out funky if we didn't do that.

@tonysyu
Copy link
Author

tonysyu commented Nov 12, 2014

Thanks for the reply! No worries, if this goes in the waste bin. I was actually refactoring because I was thinking of taking a go at the JS conversion, but I wanted to figure out how the Python code actually worked. If this part of the code is going to be tossed in the near future, feel free to close this.

@jiffyclub
Copy link
Owner

I've put up #42 with a version 0.2 release candidate, please feel free to have a look at that and see if it works for you.

@jiffyclub jiffyclub closed this Nov 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants