ENH: Profile VMS as well as RSS #2331
Conversation
effigies
changed the title
nipype/interfaces/base.py
Outdated
| 'cpus': vals[:, 2].tolist(), | ||
| 'vms_gb': (vals[:, 3] / 1024).tolist(), |
There was a problem hiding this comment.
just a nitpick here - should we make the gb be gib or GiB to maintain consistency with the term Gibibytes.
There was a problem hiding this comment.
There was a problem hiding this comment.
i think both are disruptive. since more people will use the name, i think changing the name ensures that people using it accounts for it. changing the log base implies that people using it may interpret the data incorrectly. it's possible this is already happening.
my vote would be for changing the name.
There was a problem hiding this comment.
I don't think there are a critical mass of people to maintain a name that poorly represents the concept. Let's change it to GiB before more people start using the monitor, following @effigies preference 👍 .
|
I'm leaving alone non-profiler uses of |
nipype/utils/profiler.py
Outdated
| except psutil.NoSuchProcess: | ||
| pass | ||
|
|
||
| print('%f,%f,%f' % (time(), (mem / _MB), cpu), | ||
| print('%f,%f,%f,%f' % (time(), rss / _MB, cpu, vms / _MB), |
There was a problem hiding this comment.
a little nitpick here: maybe best putting cpu right after the timestamp? - sorry I didn't suggest this in the first round.
There was a problem hiding this comment.
Sure. I was trying to leave anything that depends on the old columns usable, but I agree that I'd rather have time,cpu,rss,vms.
There was a problem hiding this comment.
let's make the change now, better than after people start using this for real.
|
Sounds very good 👍 |
Given that some systems are limited on virtual memory allocation, and not actual memory consumption, being able to profile both memory metrics would be useful