Skip to content

Fix memory leak#181

Merged
rgommers merged 27 commits intoPyWavelets:masterfrom
kwohlfahrt:master
Jul 6, 2016
Merged

Fix memory leak#181
rgommers merged 27 commits intoPyWavelets:masterfrom
kwohlfahrt:master

Conversation

@kwohlfahrt
Copy link
Member

@kwohlfahrt kwohlfahrt commented Jul 5, 2016

Fixes #180. Something about copying a temporary memoryview (or is it a memoryviewslice?) causes Cython to not free something. Not entirely sure what's going on but this fixes it and looks reasonably clean to me. If somebody with more Cython experience can cast an eye over it that'd be great, otherwise I'm merging it tomorrow morning.

Also if anybody knows of a good way to test for memory leaks, that would be nice. I think py.test supports this, but haven't looked in a while.

Edit 2: Is this a bug that should be reported to upstream Cython?

Kai Wohlfahrt and others added 25 commits July 2, 2016 11:40
Transition to flat setuptools package. "setup.py test" works, "runtests.py"
does not. Note "setup.py test" runs tests in source tree.
Try a minimal AppVeyor setup, and see what happens.
Tests now work fine with setup.py, so use that for travis.

This currently doesn't use coverage or doctests, so those are TODO.
The install step is split into two stages, the first creates wheels for all
requirements, and the second installs from those wheels. The wheels and
downloaded sources are cached with AppVeyor to reduce build times.
Debian stretch has a (very) old pip, and what worked on my machine is no
longer valid. Use new caching mechanims instead.
Cache pip install on travis-ci as well.
According to the article linked in the header, python3.3 and 3.4 need a
special flag to make sure the build environment is set up correctly.

This works together with util/appveyor/build.cmd.
This makes the build process a little more consistent (same packages updated
and installed on both platforms). Also removes the dependency of the appveyor
cache on appveyor.yml, which was preventing me from properly testing it.

Caching on Travis doesn't seem to be working, but we'll see if this helps.
Numpy build was failing, this might be the reason.
This commit enables doctests and coverage reports. Also fixes the absurdly
long logs (nosetests is more compact than test).

Note matplotlib is required for data/_readers.py, use a non-interactive
backend for travis.
Update tox.ini to install matplotlib for new doctests, and don't change out
of source directory.

Coverage isn't quite right, it includes lots of other modules not part of
pywt (dateutil, etc)
No longer used.
Apparently setup.py nosetests already specifies --where, so using that
gives a warning that it'll be deprecated in future. Use ignore-files
instead.

Install coverage on appveyor.
Shape returns e.g. (512L, 512L) on python2 appveyor. Check for equality
instead.
Other packages (notably matplotlib) link against numpy, so they must be
installed in the correct order.
Don't specify a numpy version so pip doesn't insist on upgrading it with
pywt.

Cython is treated as a compiler and not required after distribution, so
don't require it.
This allows the import of Cython to fail, and attempts to use pre-built C
sources if it does.
Tests before didn't pick up problems with the distribution (e.g. missing
data files) that were present in the source tree. This should be fixed in
CI now, developers should probably use:

./setup.py build
nosetests build/lib.xyz/ --tests pywt/tests
nosetests wasn't added to $PATH on windows, so specify full location.
Old link was broken/the wrong link in the first place.
@kwohlfahrt
Copy link
Member Author

kwohlfahrt commented Jul 5, 2016

LINK : fatal error LNK1181: cannot open input file 'Files.obj'
error: Command "C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\BIN\link.exe /nologo /INCREMENTAL:NO /LTCG /DLL /MANIFEST:EMBED,ID=2 /MANIFESTUAC:NO /LIBPATH:C:\Python35\libs /LIBPATH:C:\Python35\PCbuild\win32 /LIBPATH:C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\LIB /LIBPATH:C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\ATLMFC\LIB /LIBPATH:C:\Program Files (x86)\Windows Kits\10\lib\10.0.10586.0\ucrt\x86 /LIBPATH:C:\Program Files (x86)\Windows Kits\NETFXSDK\4.6.1\lib\um\x86 /LIBPATH:C:\Program Files (x86)\Windows Kits\10\lib\10.0.10586.0\um\x86 /EXPORT:PyInit__pywt build\temp.win32-3.5\Release\pywt\_extensions\_pywt.obj build\temp.win32-3.5\Release\pywt\_extensions\c\common.obj build\temp.win32-3.5\Release\pywt\_extensions\c\convolution.obj build\temp.win32-3.5\Release\pywt\_extensions\c\wt.obj build\temp.win32-3.5\Release\pywt\_extensions\c\wavelets.obj /OUT:build\lib.win32-3.5\pywt\_extensions\_pywt.cp35-win32.pyd /IMPLIB:build\temp.win32-3.5\Release\pywt\_extensions\_pywt.cp35-win32.lib" failed with exit status 1181
Command exited with code 1

On Appveyor Python-3.5 builds only, both 64- and 32-bit. Going to see if it builds on the setuptools branch.

@kwohlfahrt
Copy link
Member Author

OK, looks good on top of #161. That should be good to merge now too.

@rgommers
Copy link
Member

rgommers commented Jul 5, 2016

Haven't seen this issue before, but then making a copy of a temporary memoryview isn't common. Fix looks fine to me.

Doing a copy of a temporary memoryview resulted in a leak. Storing the
input shape into an explicit memoryview object and then copying that does
not cause a leak.
@kwohlfahrt kwohlfahrt mentioned this pull request Jul 6, 2016
2 tasks
@rgommers rgommers merged commit 413fac6 into PyWavelets:master Jul 6, 2016
@rgommers
Copy link
Member

rgommers commented Jul 6, 2016

Merged, thanks @kwohlfahrt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak

2 participants