Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't leak objects in _pickle_27.c load_[short_]binbytes. #37

Merged
merged 1 commit into from
May 16, 2018

Conversation

jamadden
Copy link
Member

Fixes #36.

Manually verified according to zopefoundation/ZODB#203. Here's the current memory usage of the test case described there:

leak

I'm not sure if/how to do some sort of automated regression test for this...

Copy link

@Krilivye Krilivye left a comment

Choose a reason for hiding this comment

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

Hi looking at your comment I think it's PyTuple_SET_ITEM who keeps the ref even if it fails !

src/zodbpickle/_pickle_27.c Outdated Show resolved Hide resolved
Copy link

@Krilivye Krilivye left a comment

Choose a reason for hiding this comment

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

C style review

src/zodbpickle/_pickle_27.c Outdated Show resolved Hide resolved
@azmeuk
Copy link
Member

azmeuk commented May 16, 2018

@jamadden As a regression test, what about a simple memory consumption comparison using psutil, like the one in the ZODB bug report?

@jamadden
Copy link
Member Author

I have three concerns about a simple memory consumption comparison using psutil:

  1. It's an added binary dependency. That dependency cannot be installed everywhere. For example, it cannot be installed on PyPy on Windows (Appveyor). (We don't currently test that combo, but we probably should.)
  2. There are platform differences in both the fields available from psutil (and their meaning) and the Python memory allocators. That makes writing a truly portable test that way difficult. (I'm assuming; it's not like I have [or want to] boot up various virtual machines to try...)
  3. It would necessarily be slow. It takes a large number of iterations (approximately 10,000) on my machine to see the memory stabilize, and that's in a process that's only doing the test.

gevent uses reference counting and objgraph to check for leaks, and that code is finicky. But here, objgraph didn't reliably find the dropped tuple, not with show_growth and not with show_most_common_types as in the example documentation. It's not clear to me why. (I probably did something wrong. Or maybe its because tuples containing only strings aren't necessarily tracked by the GC and hence are invisible to objgraph? I'm guessing the cases it could find---very early in the run---had to do with the tuple freelist in some fashion. @mgedmin ?)

Because there are no existing reference counting tests in this codebase, and because of the complexity outlined above, and because Python 2.7 is a "legacy" platform at this point, I'm inclined to ask for a pass on an added regression test. @icemac ?

@mgedmin
Copy link
Member

mgedmin commented May 16, 2018

maybe its because tuples containing only strings aren't necessarily tracked by the GC and hence are invisible to objgraph

That is, unfortunately, very true.

@azmeuk
Copy link
Member

azmeuk commented May 16, 2018

Because there are no existing reference counting tests in this codebase, and because of the complexity outlined above, and because Python 2.7 is a "legacy" platform at this point, I'm inclined to ask for a pass on an added regression test.

That makes sense.

@icemac
Copy link
Member

icemac commented May 16, 2018

@jamadden I'm fine with fixing this test without adding a proper test. The verification shown in this PR is enough as the change log links to it.

@jamadden
Copy link
Member Author

@icemac @mgedmin Thanks! I'm going to take that as approval to merge then.

Could we get a PyPI release (or access to do so), please?

@jamadden jamadden merged commit eb5d343 into master May 16, 2018
@jamadden jamadden deleted the issue36 branch May 16, 2018 13:30
@mgedmin
Copy link
Member

mgedmin commented May 16, 2018

@jamadden you're now a PyPI owner of zodbpickle.

@jamadden
Copy link
Member Author

@mgedmin Thanks! The new pypi.org (warehouse) sends out notification emails when that happens, so that was nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants