[PATCH] Improve profiling doc and make the data easier to visualise

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:
> Robert Collins wrote:
>> A few things..
>>
>> I'm not convinced that doing it magically is the right thing to do...
>> but if we do, surely the endswith test should be for '.txt' not 'txt'
>> etc?
>>
>> I think the logic for what to add to the output file should not be in
>> the UI layer; that should be a helper function or something in the
>> lsprof module so that it can be tested directly and reused.
>>
>> Its a shame about needing to do isinstance, that would seem to be a
>> defect in the lsprof internals? Perhaps a note in the code about what is
>> being achieved by the isinstance calls ? e.g.
>>
>> # where isinstance(code, str) is used it is to determine if the code 
>> # object is actually a code object and thus has a filename and other
>> # details we can print, or if its just a string and as such we can only
>> # print its str() representation.
>>
>> I think we are trying to split HACKING into more smaller pieces. Perhaps
>> you could add this to the developers index and give it its own file?
>>
>> +1 with these points addressed (either by telling me I'm wrong, or
>> changing things :).
>>
>> -Rob
> 
> New patch attached. I do feel that conversion to user-friendly output
> formats is a good thing. Running the profiler then looking at the data
> immediately is the common case. One ought to be able to do that without
> needing to run some helper function IMO. As requested, I've made the
> triggering for those conversions more explicit though with extensions of
>  .txt  and .callgrind being required to make it happen.
> 
> All other changes requested have been made.
> 
> Ian C.
> 
> 

A few comments.

First, I think Robert was saying that there should be a unit-testable
helper function for determining the output format. And that it should
have unit tests. Not that it was something external that you need to run
after the profile has been run.

Which I fully support.


Further, a few comments on the code:

- -        stats.freeze()
- -        cPickle.dump(stats, open(filename, 'w'), 2)
+        outfile = open(filename, 'w')
+        if filename.endswith(".callgrind"):
+            stats.calltree(outfile)
+        elif filename.endswith(".txt"):
+            stats.pprint(file=outfile)
+        else:
+            stats.freeze()
+            cPickle.dump(stats, outfile, 2)
+        outfile.close()

I always wonder when I see a file opened in 'w', versus 'wb'. I don't
know how python pickles interact with Windows, but I generally prefer to
see 'wb'.

Second, we have taken the stance of always using try/finally whenever we
open a file. It probably doesn't hurt anything here, but it is good to
stay in the habit. So you don't forget somewhere that it is important.
So it should be:

outfile = open(filename, 'wb)
try:
  if filename.endswith('.callgrind'):
    stats.calltree(outfile)
...
finally:
  outfile.close()



As an aside, I believe the reason for 'isinstance()' is because you
can't serialize code objects. So when you load things from a pickle,
they don't come back quite the same as when you put them in.


Other than that, +1 from me.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGYDpSJdeBCYSNAAMRAiFUAJ9rlvtFuGKTBmh08XYXqSM6w/X19gCgsTOM
ltvuaCRXjgegPk4bfJnt0pM=
=rUK6
-----END PGP SIGNATURE-----