Skip to content
This repository was archived by the owner on Mar 8, 2020. It is now read-only.

Fix memory leaks (thanks @amlweems) - #75

Merged
juanjux merged 2 commits into
bblfsh:masterfrom
juanjux:fix/leak
Feb 1, 2018
Merged

juanjux merged 2 commits into
bblfsh:masterfrom
juanjux:fix/leak

Conversation

@juanjux

@juanjux juanjux commented Jan 31, 2018 •

Copy link
Copy Markdown
Contributor

(Branched from amlweems:fix-memory-leak)

  • Apply the memory leaks detected and fixed by @amlweems with an aditional one for the String helper functions that allows the fix for ItemAt to work.

  • Test (also from @amlweems branch) to check that memory doesn't increase after 100 filter() calls.

Signed-off-by: Juanjo Alvarez [email protected]

Signed-off-by: Juanjo Alvarez <[email protected]>
@juanjux juanjux added the bug label Jan 31, 2018
@juanjux juanjux self-assigned this Jan 31, 2018
@juanjux
juanjux requested a review from vmarkovtsev January 31, 2018 12:52
@juanjux

juanjux commented Jan 31, 2018

Copy link
Copy Markdown
Contributor Author

cc @amlweems

@vmarkovtsev vmarkovtsev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I recommend running the test suite under valgrind and CPython configured with --without-pymalloc --with-pydebug --with-valgrind just to make sure we have not missed anything.

@amlweems

Copy link
Copy Markdown

Nice! Testing with pympler again, this fixes the major memory leak. And the objects that are still around don't scale with the number of iterations I do against filter().

# 5000 iterations of filter()
        types |   # objects |   total size
============= | =========== | ============
  <class list |        5056 |    555.20 KB
   <class str |        5045 |    438.51 KB
   <class int |        1003 |     43.10 KB
...

# 10000 iterations of filter()
        types |   # objects |   total size
============= | =========== | ============
  <class list |        5056 |    555.20 KB
   <class str |        5045 |    438.51 KB
   <class int |        1003 |     43.10 KB
...

I tried running with cpython and valgrind. I get a lot of "possibly lost", but no worrying "definitely lost". I'm not super confident in these results, but I am very happy with the object counts above. Full output attached.

# cpython build conf
./configure --without-pymalloc --with-pydebug --with-valgrind --prefix=`pwd`/build

# valgrind command
valgrind --tool=memcheck --suppressions=/tmp/cpython/Misc/valgrind-python.supp --log-file=leaks.txt --leak-check=full python -m unittest bblfsh.test.BblfshTests.testManyFilters
==21838== LEAK SUMMARY:
==21838==    definitely lost: 40 bytes in 1 blocks
==21838==    indirectly lost: 0 bytes in 0 blocks
==21838==      possibly lost: 6,314,378 bytes in 37,480 blocks
==21838==    still reachable: 193,285 bytes in 310 blocks
==21838==         suppressed: 0 bytes in 0 blocks

@juanjux

juanjux commented Jan 31, 2018 •

Copy link
Copy Markdown
Contributor Author

@vmarkovtsev that would be certainly great, I used a simpler tool that doesn't required Python to recompile called memleax, I'm pretty sure current leaks are fixed since I increased locally the iterations in the memory test to 100.000 and the memory didn't increase, but I'll give a run with Valgrind when I've a little time.

On the other side, Travis is failing with a core dump while its working perfectly on my machine :/

@vmarkovtsev

Copy link
Copy Markdown
Contributor

@amlweems This leak summary gives a solid confidence that all the leaks are fixed, awesome!

@juanjux

juanjux commented Feb 1, 2018

Copy link
Copy Markdown
Contributor Author

Found the reason why it works on my machine(TM) and not in Travis... when testing for leaks, I set the environment variable "PYTHONMALLOC" to "malloc" since that allows the memleak command to find the leaks. With that environment variable set, it works, without it, it segfaults. Looks like I'm for another debugging session 👎

@juanjux

juanjux commented Feb 1, 2018

Copy link
Copy Markdown
Contributor Author

Looks like the problem comes from mixing malloc with pymalloc in the same extension module. This is not a problem for shared libraries, but since libuast is compiled with the extension module it causes problems. The solutions I'm evaluating are requiring and using libuast.so or adding a function to libuast to pass the alloc/free functions to use.

@juanjux

juanjux commented Feb 1, 2018 •

Copy link
Copy Markdown
Contributor Author

Ok, the pointer-between-different-mallocs problem is hellish and using the libuast.so crashed just the same, so I did a workaround that while not perfect will avoid the leak: I store the problematic PyObjects in a PyList and free (DECREF) it at the end of Python's filter() wrapper when libuast is not touching them anymore for sure.

@juanjux
juanjux merged commit 3b71f52 into bblfsh:master Feb 1, 2018
@vmarkovtsev

Copy link
Copy Markdown
Contributor

Amazing detective work @juanjux !

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants