classification
Title: 2to3 Slight Patch
Type: performance Stage:
Components: 2to3 (2.x to 3.x conversion tool) Versions: Python 2.6
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: collinwinter Nosy List: collinwinter, nedds
Priority: normal Keywords:

Created on 2008-06-23 17:25 by nedds, last changed 2008-08-21 22:27 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
pytree.py nedds, 2008-06-23 17:25 new version of pytree.py
Messages (5)
msg68641 - (view) Author: Nick Edds (nedds) Date: 2008-06-23 17:25
This is a small patch to the 2to3 tool, replacing some calls to
isinstance (x,y) with type(x) is y in the file pytree.py. Although there
is only a slight performance increase for each time this change is made,
the recursive nature of pattern matching means that isinstance was being
calling hundreds of thousands of times per file, so overall these minor
changes result in a 3-4% speed increase in my limited testing. It
currently fails only one test due to line 432 in test_pytree, because I
had to rename the type parameter to new_type so I could call type() on
something. But with the line in the test file changed, it passes all tests.
msg68692 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2008-06-24 15:54
I'm not wild about making this change for such a minimal improvement
(which I would guess falls within the margin of error), since it will
limit extensibility and testability. I think I'd be fine with it if the
benefit were >10%, but 3% doesn't sound significant enough.
msg68694 - (view) Author: Nick Edds (nedds) Date: 2008-06-24 16:11
I don't think the improvement falls in margin of error, but maybe I'm
wrong. The problem was that even when just run on a single file,
isinstance was being called upwards of 100,000 times. I guess it doesn't
merit inclusion on its own though, but with other things I'm working on,
I think it makes sense.
msg69632 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2008-07-14 00:20
So, revisiting this...

On the face of it, I'm not convinced that the isinstance(x, Leaf) ->
type(x) is Leaf changes are correct: certain fixers have in the past
utilized their own subclasses of Node, and I can foresee this being done
again in the future. Reverting those changes seems to remove the speedup
you observed.
msg69633 - (view) Author: Nick Edds (nedds) Date: 2008-07-14 00:34
Fair enough. I guess that even though there's a little bit of a
performance improvement from this, it does hurt extensibility, so its
probably not a worthwhile change after all.
History
Date User Action Args
2008-08-21 22:27:30benjamin.petersonsetstatus: open -> closed
resolution: rejected
2008-07-14 00:34:27neddssetmessages: + msg69633
2008-07-14 00:20:30collinwintersetmessages: + msg69632
2008-06-24 16:11:17neddssetmessages: + msg68694
2008-06-24 15:54:32collinwintersetmessages: + msg68692
2008-06-23 17:25:43neddscreate