Hi Robin & Gordon et al,
Since Gordon comp^H^H^H^H expressed his concern about loss of speed for the
basic list of tuples case, I went back and reworked the patch starting with
the FAST version I included in my previous email. The new patch is either
faster or essentially unchanged for every type of input I could think of to
throw at it. The change most likely to concern people is that [(anInstance0,
anInstance1),...] no longer works. This is part of a trade off of safety,
speed and flexibilty that I discuss below.
The patch is against wxPython-2.2.5. The patch should be applied by going to
the src directory and running "patch -Np1 < wxhelper-2.patch". Swig will
need to be rerun since I modified my_typemaps.i.
It allows wxPoint_LIST_Helper to accept any sequence of length-2 sequences
instead of just lists of tuples. It also plugs a couple of memory leaks and
catches nonnumeric objects. I discuss performance, API, memory leak
implications of the patch below.
Not too suprisingly this turned out to be somewhat more complicated than I
had originally thought. I had to mess with my_typemaps.i and helpers.h in
addition to helpers.cpp. Since I know very little about Swig, (although much
more than I knew yesterday), it would be good to look over my changes to
my_typemaps.i to make sure they make sense.
SAFETY vs. SPEED vs. INSTANCES
Instances can call arbitrary code when methods are invoked on them. This
means in particular that PyInt_AsLong(anInstance) can run arbitrary python
code. This make is hard (impossible?) to write code that is both safe and
fast. See 'theVictimList' in testwxpatch.py for an example that crashes
wxPython-2.2.5. For this reason I banned instances masquerading as numbers
in the patch (UserInt for example).
Thus, under the patch, the following are legal:
[[1,2],[2,3]]
[(1,2),(2,3)]
([1,2],[2,3])
[wxPoint(1,2), wxPoint(2,3)]
myInstanceWhichReturnsTwoTuplesFromGetitem
Numeric.array([[1,2],[2,3]])
While this is not:
[(anInstance,anInstance), (anInstance), (anInstance)]
Before the patch, the following were legal:
[(1,2),(2,3)]
[wxPoint(1,2), wxPoint(2,3)]
[(anInstance,anInstance), (anInstance), (anInstance)]
Is not going to be able to do the later going to cause anyone a problem? I
would hope not, but if it does please let me know. I can remove the checks
for instances in the patch. This will make things a bit less safe, but no
worse than the situation now.
API:
I changed the API for wxPoint_LIST_Helper. It now takes a second argument in
which the sequence length is returned. This prevents evil code (see the
Pathological class in testwxpatch.py) from changing its mind about what
length it is. Without this change, there is a chance that Pathological or
its brethren could crash the interpreter. However, if you think c-code from
outside of wxPython is likely to be calling into wxPoint_LIST_Helper, then
probably it should be renamed wxPoint_SEQUENCE_Helper and a version that
maintains the old API should be included as wxPoint_LIST_Helper.
This probably wouldn't be a bad idea in any event, but I was reluctant to
mess with Swig much since I really don't know what I'm doing when it comes
to Swig.
I also changed both wxPoint_Helper and wxPoint_LIST_Helper to catch
nonnumerical values. In the past one could pass a tuple of strings where a
wxPoint was expected and it would "work". On my machine the strings were
interpreted as zeros, but I'm not sure that that is consistent across
different platforms. This now raises an error.
MEMORY LEAKS:
I fixed a memory leak in wxPoint_Helper: PySequence_GetItem returns a new
reference and o1 and o2 were not getting DECREFed. There are memory leaks in
most (all?) of the *_LIST_Helper functions as well: when an error is raised,
the temp array is not deallocated on the way out. I didn't want to touch too
much code at once, and these are pretty minor, so I left them alone (except
in wxPoint_LIST_Helper).
Setting testleaks to 1 in testwxpatch and then running it while watching the
memory used by the process (e.g. with top) will reveal the memory leak in
the old code. The memory use by the new code should be stable.
PERFORMANCE:
The patched version is equal in speed or faster in every case with the minor
exception of wxPoint which is about 1% slower. The array case, which is why
I'm doing this, is now ten time faster than the way I had to do it before.
Table: Times for plotting 50,000 points using PlotLines (see
testwxpatch.py):
array: 0.089 (0.900)
[(x,y)]: 0.050 0.084
[[x,y]]: 0.054 (0.724)
((x,y)): 0.050 (0.143)
intarray: 0.077 (0.812)
[(intx,inty)]: 0.036 0.036
[[intx,inty]]: 0.040 (0.763)
((intx,inty)): 0.036 (0.094)
[wxPoint]: 0.150 0.149
Values in parentheses were computed by first using map(tuple, data) since
these wouldn't work directly with the old version.
SUMMARY:
Well that's about it. Let me know if you think that wxPoint_LIST_Helper
needs to be renamed to keep the API consistent. Or anything else that I
would need to do make the patch acceptable.
Regards,
-tim
testwxpatch.py (4.8 KB)
wxhelper-2.patch (7.08 KB)