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
Unbounded memory growth resizing split-table dicts #72334
Comments
There is a memory leak in the new dictionary resizing in 3.6, which can cause memory exhaustion in just a few iterations. I don't fully understand the details of the bug, but it happens when resizing a dict with a split table several times. The only way that I have found to trigger this is by popping items off of an object's I've attached a script to illustrate the issue. Be careful with it, because it will eat up all your memory if you don't interrupt it. |
This patch fixes the memory leak in split-dict resizing. Each time dict_resize is called, it gets a new, larger size Set the lower bound at minused (inclusive), rather than exclusive, so that the size does not continue to increase for repeated calls. A test is added to test_dict.py based on the earlier test script, but if someone has a simpler way to trigger the split-dict resize events, I'd be happy to see it. |
Can you wrap the test with @support.cpython_only decorator? The patch fixes the memory leak demonstrated in test-dict-pop.py. |
I can add the cpython_only decorator, but I'm not sure it is the right thing to do. I would expect the code in the test to pass on any Python implementation, which would suggest that it should not be cpython_only, right? If you still think so, I'll add it. |
I don't understand the leak yet.
dictresize() is called for converting split table to combined table. In your test code, which loop cause leak? new instance loop or re-use instance loop? |
The CPython test suite uses a counter on memory allocations. Please add an unit test which triggers the memory leak, but you don't need many iterations. One iteartion should be enough to be catched by the unit test. Try: ./python -m test -R 3:3 test_dict. The memory allocation counter: Note: The tracemalloc module can also be used to track memory leak, but it's harder to use it to write unit tests because Python uses a lot of internal caches and tracemalloc really tracks every single bytes. |
Ah, is the leak happen in 3.6b1? Regardless the leak, I agree that grow dict when popping is bad idea. |
every
Both loops cause the leak. If the Does anyone have a handy way to create a split-table dict other than on
I should not have used the term memory leak, and have updated the title to be more precise. It is not memory allocated without a corresponding free, instead it is unbounded growth of the memory owned by a split-table dict. Cleaning up the object does indeed clean up the memory associated with it. The included test exercises the bug with several iterations. Running the test several times with only one iteration would not exercise the bug. |
Hum, this PEP is now probably outdated since Python 3.6 beta 1 :-) |
The leak happens in 3.6b1 and master as of an hour ago (git: 3c06edf, hg:r103797) |
I pulled just now and saw changes in dictobject.c, and just wanted to confirm the memory growth bug is still in changeset 56294e03ad89 (I think I used the right hash, this time). |
I confirmed and investigated it. Thanks! |
Please wait a second. The cause of this problem is that attribute setting causes a combined table to be splitted (the code path is [0]->[1]->[2]->[3]). So every time you set an attribute and then pop, it will do dictresize(right now it will increase memory usage). So even if you make pop() not increase memory usage, the example Min gives will still do much work not wanted (dictresize and make_keys_shared). Actually I think memory usage increasing is not a problem if other things are right. popitem() before does the same thing and should never cause a problem. [0] https://hg.python.org/cpython/file/tip/Python/ceval.c#l2248 |
Ahh, this seems not true. Test it in py3.5 also crash. |
This issue is caused by dictresize() and _PyObjectDict_SetItem()
As I wrote before, pop, popitem, and del should not increase key size. And _PyObjectDict_SetItem shouldn't convert the dict to split-table when the dict doesn't share keys |
xiang is right. Python 3.5 has same issue when using popitem(). |
This is patch for Python 3.5. |
LGTM. But why this change? -#define ESTIMATE_SIZE(n) (((n)*3) >> 1) |
xiang: If there are any integer a such as ESTIMATE_SIZE(a) == n and n == 2**m and USABLE_FRACTION(n) == a - 1, This is why ESTIMATE_SIZE should round up fraction. |
Ahh, I see.
There are, such as 11, 43...
It can but needs another resize in insertdict which breaks the intention of ESTIMATE_SIZE. Then everything looks fine to me. :) |
haypo, Could you review fix-28147-py35.patch and fix-28147.patch ? Draft NEWS entry:
|
This issue seems to have slipped through. Should it be a release blocker for 3.6.0 final or can it wait for 3.6.1? |
Calling pop() for instance's __dict__ is not common operation. I expected that this bug does not affect any real code. But the case in bpo-28894 looks as a real case. This might be a release blocker. |
On Python 3.5, instance.__dict__.popitem() cause this issue. This is not new issue, but there is small regression. I don't know this should be fixed in 3.6.0. |
In this case, I suggest to wait for 3.6.1 to fix it. |
Just follow the normal 3.6 branch which will ensure it gets into 3.6.1. If we end up deciding to also add it to 3.6.0 final, I will handle the cherrypicking. |
Here is patch for Python 3.6. |
http://bugs.python.org/issue28894 was duplicate of this issue. Since real world program suffered by this, I'm +1 to fix this by 3.6.0 |
I think these proposed patches need careful review but, even so, given the extent of the fix28147-py36-2.patch changes, I don't think they should go into 3.6.0 at the last minute. Unless somebody can convince me otherwise, I'm going to let this wait for 3.6.1. |
Hi, pybind11 (https://github.com/pybind/pybind11) dev here. We're seeing massive memory increases due to this bug, which completely break our test suite (and likely any use of this library, which is commonly used to bind C++ code to Python). Please look at the following issue ticket: where RSS goes to 43MB to 4.3GB for the basic set of tests. The fancy fancy test suite which also covers NumPy/SciPy can't even be run anymore. All issues disappear when the dict patch listed here is applied. We're really concerned that this is not slated to be fixed for 3.6.0. Pybind11 is not doing anything particularly special with dicts -- if this is hitting us, others will likely have issues as well. -Wenzel |
It appears this issue has stalled again. Is the most recent patch fix28147-py36-2.patch) ready for review? If so, can someone please review it? Then it needs to be pushed to the 3.6 branch for 3.6.1 and exposed to the buildbots. Only then could we consider cherrypicking for 3.6.0 and a change of the magnitude in the patch would require at least another preview release and delay 3.6.0 final. 3.6.0 is scheduled to be released tomorrow. I am very reluctant to delay now for an issue that has been open now for 3 months throughout the beta phase of the release cycle. If enough people feel we should delay 3.6.0 to address this, we can. But without a fix reviewed and committed and without more feedback from other core developers, 3.6.0 is going out without it. @inada.naoki ? @Haypo ? @serhiy.storchaka ? Others? |
fix28147-py36-2.patch: test_dict pass with DEBUG_PYDICT defined. |
I reviewed fix28147-py36-2.patch, I have some requests. |
Ned: "If so, can someone please review it?" Done. Ned: "I am very reluctant to delay now for an issue that has been open now for 3 months throughout the beta phase of the release cycle. If enough people feel we should delay 3.6.0 to address this, we can." The bug was seen as minor and I didn't expect that anyone would notice it. Wenzel Jakob wrote "RSS goes to 43MB to 4.3GB": for me, it's a major regression, and it's not possible to work around it. With such bug, Python 3.6 is unusable on such application (pybind11). I easily imagine that such memory leak is a blocker issue for a server running for days. I now consider that Python 3.6.0 must not be released with such blocker issue. |
fix28147-py36-3.patch LGTM: Naoki, please push it right now. But please see also my comments on fix28147-py36-2.patch: I would still apprecicate that we enhance the comment in dictobject.c. The comment is not a blocker issue for 3.6.0, it can be enhanced later :-D |
This patch updates the comment. |
New changeset 85be9dcc16a8 by Victor Stinner in branch '3.6': |
I dislike pushing a change written by another core dev, I prefer that he/she directly push it, but we are out of time. Ned asked to first push to Python 3.6 to get a confirmation of buildbots, before being able to ask for a cherry-pick in 3.6.0 final. Anyway, thanks for the fix Naoki! |
I think this can be tested without _testcapi. See an example in bpo-28894. |
I asked a question about the change in _PyObjectDict_SetItem. It is not clear to me. |
New changeset b70b2d3f3167 by Victor Stinner in branch '3.6': |
[cherrypicked for 3.6.0rc2] |
Oh, I'm sorry. It seems I had failed to push the commit yesterday. |
No problem. Thanks for your fix! I prefer to not see the bug in Python |
So now that a fix has been released with 3.6.0rc2, what else needs to be done to close this issue? Why is 3.5 selected in the applicable versions? |
Before Python 3.6, instance.__dict__.pop(key) didn't trigger this. class C: pass
a = C()
a.a = 1
while True:
a.__dict__.popitem() # convert split table into combined table.
a.a = 1 # convert combined table into split table again. |
The 3.5 patch LGTM. |
New changeset cc40470c10f8 by INADA Naoki in branch '3.5': |
Misc/NEWS
so that it is managed by towncrier #552Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: