gprof2dot icon indicating copy to clipboard operation
gprof2dot copied to clipboard

Add units to timings in node labels

Open robinst opened this issue 7 years ago • 4 comments

Currently, the timings on node labels are shown like this: 0.1234

It's not immediately clear that that's in seconds. Can we add a " s" to the label? Or maybe even consider changing it to milliseconds and adding " ms".

I'm not familiar with all the input formats and if they have the timing information in seconds or not, but if it's just a matter of changing the label formatting, I'm happy to raise a PR.

robinst avatar Feb 22 '19 06:02 robinst

I agree having a time unit (ms in particular) would be better.

But I also don't know off hand if time units are being consistently parsed for all formats. This is because until recently we didn't care about exact time units, as we always computed the relative time (against total.)

I'm afraid I don't have much time to look into this myself, but I'd accept patches.

CC'ing @robinst

jrfonseca avatar Feb 22 '19 12:02 jrfonseca

But I also don't know off hand if time units are being consistently parsed for all formats

Would it be acceptable to make this dependent on the format? I'm currently using pstats and I think it always shows times in seconds. So if you use pstats, you'd get units in labels but if you use another format you would get what you see now (just a number).

robinst avatar Feb 25 '19 03:02 robinst

I think it might be easier to apply a (format-specific) conversion factor when parsing the stats, than to carry around the unit. I'm OK with storing internally all times in seconds, and then perhaps show in ms for times less than 1 second.

That is, my suggestion is to presume for now all formats represent time in seconds, and then fixup later the parsing of formats that don't.

jrfonseca avatar Feb 25 '19 07:02 jrfonseca

My idea was to just add the "ms" if we're certain about what unit a time is in. But if you're ok with potentially showing the wrong unit for some formats, I can raise a PR for that.

robinst avatar Feb 26 '19 23:02 robinst