Repository navigation
GC operates out of global runtime state. #81035
Description
Note: 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:
assignee = 'https://github.com/ericsnowcurrently'
closed_at = <Date 2019-11-22.13:00:22.999>
created_at = <Date 2019-05-08.17:00:25.446>
labels = ['expert-subinterpreters', '3.9']
title = 'GC operates out of global runtime state.'
updated_at = <Date 2022-01-12.23:02:12.811>
user = 'https://github.com/ericsnowcurrently'bugs.python.org fields:
activity = <Date 2022-01-12.23:02:12.811>
actor = 'vstinner'
assignee = 'eric.snow'
closed = True
closed_date = <Date 2019-11-22.13:00:22.999>
closer = 'vstinner'
components = ['Subinterpreters']
creation = <Date 2019-05-08.17:00:25.446>
creator = 'eric.snow'
dependencies = []
files = []
hgrepos = []
issue_num = 36854
keywords = ['patch']
message_count = 18.0
messages = ['341911', '357048', '357053', '357061', '357062', '357150', '357151', '357160', '357263', '357269', '357273', '357275', '357276', '357328', '357329', '357795', '357796', '358355']
nosy_count = 5.0
nosy_names = ['vstinner', 'phsilva', 'eric.snow', 'pablogsal', 'miss-islington']
pr_nums = ['13219', '17279', '17285', '17287', '17331', '17338', '17339', '17455', '30564', '30565', '30566']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue36854'
versions = ['Python 3.9']
Activity
ericsnowcurrently commentedon May 8, 2019
(also see bpo-24554)
We need to move GC state from _PyRuntimeState to PyInterpreterState.
vstinner commentedon Nov 20, 2019
New changeset 9da7430 by Victor Stinner in branch 'master':
bpo-36854: Clear the current thread later (GH-17279)
9da7430
vstinner commentedon Nov 20, 2019
New changeset 67e0de6 by Victor Stinner in branch 'master':
bpo-36854: gcmodule.c gets its state from tstate (GH-17285)
67e0de6
vstinner commentedon Nov 20, 2019
New changeset 7247407 by Victor Stinner in branch 'master':
bpo-36854: Move _PyRuntimeState.gc to PyInterpreterState (GH-17287)
7247407
vstinner commentedon Nov 20, 2019
It's now done in the future Python 3.9!
vstinner commentedon Nov 21, 2019
I reopen the issue, the change introduced a reference leak :-( Example:
$ ./python -m test -R 3:3 test_atexit -m test.test_atexit.SubinterpreterTest.test_callbacks_leak 0:00:00 load avg: 1.12 Run tests sequentially 0:00:00 load avg: 1.12 [1/1] test_atexit beginning 6 repetitions 123456 ...... test_atexit leaked [3988, 3986, 3988] references, sum=11962 test_atexit leaked [940, 939, 940] memory blocks, sum=2819 test_atexit failed== Tests result: FAILURE ==
1 test failed:
test_atexit
Total duration: 466 ms
Tests result: FAILURE
It seems like each _testcapi.run_in_subinterp("pass") call leaks 3988 references.
I tried tracemalloc to see where the memory allocation are done, but tracemalloc reports a single Python line: the _testcapi.run_in_subinterp() call...
I tried to follow the increase of references using a watchpoint in gdb on _Py_RefTotal, but it takes a lot of time to follow each Py_INCREF/Py_DECREF knowning that we are talking aobut around 4,000 references.
vstinner commentedon Nov 21, 2019
Even if the test is simplified to the following code, it does still leak:
vstinner commentedon Nov 21, 2019
The following patch fix it:
Py_NewInterpreter() indirectly calls "import encodings" which calls codecs.register(search_function). This encodings function is stored in interp->codec_search_path and so keeps encodings module dict alive.
_PyImport_Cleanup() removes the last reference to the encodings *module*, but the module deallocator function (module_dealloc()) doesn't clear the dict: it only removes its strong reference to it ("Py_XDECREF(m->md_dict);").
interp->codec_search_path is cleared by PyInterpreterState_Clear() which is called by Py_EndInterpreter(). But it is not enough to clear some objets. I'm not sure if encodings module dict is still alive at this point, but it seems like at least the sys module dict is still alive.
I can push my workaround which manually "break a reference cycle" (really? which one?), but I may be interested to dig into this issue to check if we cannot find a better design.
_PyImport_Cleanup() and _PyModule_Clear() functions are fragile. They implement smart heuristics to attempt to keep Python functional as long as possible *and* try to clear everything. The intent is to be able to log warnings and exceptions during the Python shutdown, for example.
The problem is that the heuristic keeps some objects alive longer than expected. For example, I would expect that _PyImport_Cleanup() not only calls sys.modules.clear(), but also clears the dict of each module (module.__dict__.clear()). It doesn't, and I'm not sure why.
vstinner commentedon Nov 22, 2019
New changeset 310e2d2 by Victor Stinner in branch 'master':
bpo-36854: Fix refleak in subinterpreter (GH-17331)
310e2d2
vstinner commentedon Nov 22, 2019
I'm not fully happy with this solution, but at least, it allows me to move on to the next tasks to implement subinterpreters like PR 17315 (bpo-38858: Small integer per interpreter).
vstinner commentedon Nov 22, 2019
New changeset 84c36c1 by Victor Stinner in branch '3.8':
bpo-36854: Fix reference counter in PyInit__testcapi() (GH-17338)
84c36c1
miss-islington commentedon Nov 22, 2019
New changeset bff5255 by Miss Islington (bot) in branch '3.7':
bpo-36854: Fix reference counter in PyInit__testcapi() (GH-17338)
bff5255
vstinner commentedon Nov 22, 2019
I close again the issue ;-)
ericsnowcurrently commentedon Nov 22, 2019
Thanks so much for getting this done, Victor!
Should we have an issue open for finding a better solution? Are there risks with what you did that we don't want long-term?
ericsnowcurrently commentedon Nov 22, 2019
Did I mention that you're my hero? :)
vstinner commentedon Dec 4, 2019
Victor> I'm not fully happy with this solution
Eric> Should we have an issue open for finding a better solution? Are there risks with what you did that we don't want long-term?
Pablo made a small changes in my workaround, by calling _PyGC_CollectNoFail() after PyInterpreterState_Clear().
https://github.com/python/cpython/pull/17457/files
I tried to avoid that, since I consider that no arbitrary Python code should be called after PyInterpreterState_Clear(), whereas the GC can trigger arbitrary __del__() methods implemented in pure Python. See discussion at #17457
Each time I tried to fix a bug in the Python finalization, I introduced worse bugs :-D
We cannot fix all bugs at once, we have to work incrementally. I like the idea of introducing workarounds specific to subinterpreters: leave the code path for the main interpreter unchanged. It helps to iterate on the code to slowly fix the code.
I prefer to not open an issue, since the Python finalization is broken is so many ways :-D Anyway, I'm hitting issues on the finalization each time I'm working on subinterpeter changes, so it's hard to forget about it :-)
I started to take notes at:
https://github.com/python/cpython/pull/17457/files
vstinner commentedon Dec 4, 2019
Eric:
"""
Thanks so much for getting this done, Victor!
Did I mention that you're my hero? :)
"""
You're welcome. I'm a believer that subinterpreters is one of the most realistic solution to make Python faster. I said it in my EuroPython keynote on CPython performance ;-)
https://github.com/vstinner/talks/blob/master/2019-EuroPython/python_performance.pdf
"Conclusion: PyHandle, tracing GC and subinterpreters are very promising!"
ericsnowcurrently commentedon Dec 13, 2019
On Wed, Dec 4, 2019 at 4:36 AM STINNER Victor <report@bugs.python.org> wrote:
:)
+1
+1
Sounds good. :)
On Wed, Dec 4, 2019 at 4:39 AM STINNER Victor <report@bugs.python.org> wrote:
Again, thanks for that!