Hi Robin,
Here's the patch to helpers.cpp that I promised. The patch is against
wxPython-2.2.5. The patch should be applied by going to the src directory
and running "patch -Np1 < wxhelper.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.
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:
There was some question about how using the general sequence protocol would
affect speed. As shown by the data below, there is a signifigant impact: for
the best case (a list of tuples of ints) things slowed down by about 50%.
However the speed of plotting Numeric arrays is improved by over a factor of
four. The slowdown for the lists of tuples cases can be eliminated by adding
special cases to the code, but at the expense of complexity. I suspect that
most applications that are plotting large numbers of points use Numeric, so
I think that the first and fourth rows in the table below are most
important. (This is obviously a somewhat biased view). I personally don't
think the complexity that the fast version of the codes add is worth the
speed gain, so the patch is against the simpler new version. NEW-FAST is
included with this message as a separate file, but it has not been tested or
even scrutinized very much.
Table: Times for plotting 50,000 points using PlotLines (see
testwxpatch.py):
TEST NEW NEW-FAST OLD
array: 0.134961007614 0.134145037987
(0.634264634192)
[(x,y)]: 0.104322197374 0.0881805369118
0.0870843082012
[[x,y]]: 0.108765723018 0.103665577608
(0.613360413125)
intarray: 0.086473224949 0.0866711830694
(0.626794075783)
[(intx,inty)]: 0.0548769187145 0.0394333916741
0.0387480531744
[[intx,inty]]: 0.0564195728787 0.0531457489709
(0.462688805421)
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
P.S. I suspect this probably showed up on the mailing list already when I
wasn't paying attention, but I had to change the indent level of the wxc
target in the setup.py file to get wxc to compile (on nt). If it hasn't
shown up already, or you don't know what I'm talking about, let me know and
I'll supply some details.
P.P.S In order to get the generated swig files to compile (NT) I had to run
the attached swigfixer.py program over them. Anyone else run into this
issue?
swigfixer.py (359 Bytes)
fast_wxPoint_LIST_Helper.cpp (2.46 KB)
wxhelper.patch (6.42 KB)
testwxpatch.py (4.16 KB)