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

Added windows support #1

Merged
merged 2 commits into from
Apr 27, 2012
Merged

Added windows support #1

merged 2 commits into from
Apr 27, 2012

Conversation

orf
Copy link

@orf orf commented Apr 25, 2012

The psutil module can be used to make memory_profiler cross platform. This patch does this, falling back to the original subprocess method if psutil is not available.

orf added 2 commits April 25, 2012 14:13
… falling back to the original subprocess method if psutil is not installed.
@fabianp
Copy link
Collaborator

fabianp commented Apr 25, 2012

I did not know about psutil. Looks really nice!

@orf
Copy link
Author

orf commented Apr 25, 2012

It is! A lot of code that relies on subprocess and UNIX tools can make use of it instead. Please consider merging this, I haven't tested it on a Linux system but I am assuming it works as expected.

@fabianp
Copy link
Collaborator

fabianp commented Apr 25, 2012

Great. I'm just busy with other things right now but the code is fine
and will merge ASAP

On 4/25/12, orf
[email protected]
wrote:

It is! A lot of code that relies on subprocess and UNIX tools can make use
of it instead. Please consider merging this, I haven't tested it on a Linux
system but I am assuming it works as expected.


Reply to this email directly or view it on GitHub:
fabianp#1 (comment)

raise NotImplementedError('Only UNIX supported at the moment')
process = psutil.Process(pid)
return float((float(process.get_memory_info()[0]) / 1024)) / 1024
except ImportError:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same as float(process.get_memory_info()[0]) / (1024 ** 2) ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, and that way looks a lot nicer as well, or though the output is still the same. Your way is also twice as fast according to the timeit module.
Also initiating a Process object is fairly expensive (which I was not expecting):

>>> timeit.Timer("p = psutil.Process(1344)", "import psutil").timeit()
5.5183132777421235

It would probably be best to only create it once. I can send a new pull request if you want, or change it after its been merged? It will still be faster than launching "ps" every time _get_memory() gets called though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, OK great. I'll make those trivial changes.

I'd like to include you in a THANKS section, what name should I use ? do you have a webpage?

Cheers,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, Use Tom. My website is http://tomforb.es

Thanks!

@fabianp fabianp merged commit da98179 into pythonprofilers:master Apr 27, 2012
@fabianp
Copy link
Collaborator

fabianp commented Apr 27, 2012

It's in. thanks!

fabianp pushed a commit that referenced this pull request Aug 30, 2020
making mprof.py compatible with python 2.6
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.

2 participants