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

fix memory leak: add Py_DECREF to non-returned owned references - #74

Closed
amlweems wants to merge 2 commits into
bblfsh:masterfrom
amlweems:fix-memory-leak
Closed

fix memory leak: add Py_DECREF to non-returned owned references#74
amlweems wants to merge 2 commits into
bblfsh:masterfrom
amlweems:fix-memory-leak

Conversation

@amlweems

@amlweems amlweems commented Jan 28, 2018 •
edited
Loading

Copy link
Copy Markdown

There are a few memory leaks in pyuast.c when objects are owned by a function, used, and not returned. I've been doing some benchmarking and here's a comparison of memory usage / objects. I've also added a unit test which can be used to test for memory leaks in the filter function (for now, it just considers memory usage doubling to be a leak).

Note: these numbers are based on a patched libuast which fixes a different leak.

Before

Iterations: 500
Memory ratio: 8.742483577564427
        types |   # objects |   total size
============= | =========== | ============
  <class list |     4128542 |    467.09 MB
   <class str |        8099 |    528.04 KB
  <class dict |         560 |    120.05 KB

After

Iterations: 500
Memory ratio: 1.0705486410972822
        types |   # objects |   total size
============= | =========== | ============
  <class list |        5542 |    507.60 KB
   <class str |        5093 |    363.70 KB
  <class dict |         560 |    120.05 KB

Memory stats obtained via pympler:

from pympler.tracker import SummaryTracker
tracker = SummaryTracker()
# work()
tracker.print_diff()

@amlweems

amlweems commented Jan 29, 2018

Copy link
Copy Markdown
Author

Oof, I've done some more testing and it's trickier than I thought. The call to Py_DECREF in ItemAt causes a segfault on Linux. I've removed that, which brings back one of the larger leaks (as seen in the Travis CI test, a memory leak ratio of 4.58 is no bueno).

It seems like this may be difficult to solve, given that libuast ingests data from Python but cannot mark references as decremented.

@juanjux

juanjux commented Jan 29, 2018

Copy link
Copy Markdown
Contributor

Hi,

Thank for the PR. Fortunately we recently added an optional callback to libuast iterator's constructor to allow to pass a binding specific callback for things like increasing and decreasing refs (currently only the Scala client is using it). Adding it also to filter is also planned and should solve this problem.

@amlweems

amlweems commented Jan 29, 2018

Copy link
Copy Markdown
Author

Nice! That's awesome to hear.

I think the other DECREFs in the PR might be useful even with the callback, but I'm not 100% sure. Certainly the largest memory leak is solved by the callback.

@juanjux

juanjux commented Jan 30, 2018

Copy link
Copy Markdown
Contributor

Found the leak. It's here: https://github.com/bblfsh/libuast/blob/master/src/uast.cc#L504

The CreateXMLNode function converts the children to XML nodes but never frees them (commenting out that the leaks goes away, even without the last DecRef commit). Currently searching for the best approach to fix it.

@juanjux juanjux mentioned this pull request Jan 31, 2018
Merged
@juanjux

juanjux commented Jan 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

@amlweems: Fixed it, it wasn't easy to track... there was another leak on the String() helper function. Fixing that one has the nice effect that now the DECREF fix for ItemAt you provided also work. I created another PR with my changes crediting this one, so I'll close it, thanks a lot for the help!

#75

@juanjux

juanjux commented Jan 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

@amlweems PS: I can't add you to the reviewers box on the other PR because you're not added to the bblfsh project, but please feel free to review it.

@juanjux juanjux closed this Jan 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

See More

Successfully merging this pull request may close these issues.

2 participants

@amlweems @juanjux