classification
Title: GC operates out of global runtime state.
Type: Stage: resolved
Components: Subinterpreters Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.snow Nosy List: eric.snow, miss-islington, pablogsal, phsilva, vstinner
Priority: normal Keywords: patch

Created on 2019-05-08 17:00 by eric.snow, last changed 2020-05-15 00:37 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 13219 closed eric.snow, 2019-05-09 15:38
PR 17279 merged vstinner, 2019-11-20 09:45
PR 17285 merged vstinner, 2019-11-20 10:23
PR 17287 merged vstinner, 2019-11-20 11:03
PR 17331 merged vstinner, 2019-11-21 23:13
PR 17338 merged vstinner, 2019-11-22 11:30
PR 17339 merged miss-islington, 2019-11-22 12:39
PR 17455 closed pablogsal, 2019-12-03 21:45
Messages (18)
msg341911 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-05-08 17:00
(also see #24554)

We need to move GC state from _PyRuntimeState to PyInterpreterState.
msg357048 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-20 10:17
New changeset 9da7430675ceaeae5abeb9c9f7cd552b71b3a93a by Victor Stinner in branch 'master':
bpo-36854: Clear the current thread later (GH-17279)
https://github.com/python/cpython/commit/9da7430675ceaeae5abeb9c9f7cd552b71b3a93a
msg357053 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-20 10:48
New changeset 67e0de6f0b060ac8f373952f0ca4b3117ad5b611 by Victor Stinner in branch 'master':
bpo-36854: gcmodule.c gets its state from tstate (GH-17285)
https://github.com/python/cpython/commit/67e0de6f0b060ac8f373952f0ca4b3117ad5b611
msg357061 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-20 11:25
New changeset 7247407c35330f3f6292f1d40606b7ba6afd5700 by Victor Stinner in branch 'master':
bpo-36854: Move _PyRuntimeState.gc to PyInterpreterState (GH-17287)
https://github.com/python/cpython/commit/7247407c35330f3f6292f1d40606b7ba6afd5700
msg357062 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-20 11:27
It's now done in the future Python 3.9!
msg357150 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-21 10:15
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.
msg357151 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-21 10:17
Even if the test is simplified to the following code, it does still leak:

    def test_callbacks_leak(self):
        _testcapi.run_in_subinterp("pass")
msg357160 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-21 11:47
> test_atexit leaked [3988, 3986, 3988] references, sum=11962

The following patch fix it:

diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
index 7591f069b4..f088ef0bce 100644
--- a/Python/pylifecycle.c
+++ b/Python/pylifecycle.c
@@ -1210,6 +1210,15 @@ finalize_interp_clear(PyThreadState *tstate)
 {
     int is_main_interp = _Py_IsMainInterpreter(tstate);
 
+    _PyImport_Cleanup(tstate);
+
+    /* Explicitly break a reference cycle between the encodings module and XXX */
+    PyInterpreterState *interp = tstate->interp;
+    Py_CLEAR(interp->codec_search_path);
+    Py_CLEAR(interp->codec_search_cache);
+    Py_CLEAR(interp->codec_error_registry);
+    _PyGC_CollectNoFail();
+
     /* Clear interpreter state and all thread states */
     PyInterpreterState_Clear(tstate->interp);
 
@@ -1640,7 +1649,6 @@ Py_EndInterpreter(PyThreadState *tstate)
         Py_FatalError("Py_EndInterpreter: not the last thread");
     }
 
-    _PyImport_Cleanup(tstate);
     finalize_interp_clear(tstate);
     finalize_interp_delete(tstate);
 }



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.
msg357263 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-22 09:58
New changeset 310e2d25170a88ef03f6fd31efcc899fe062da2c by Victor Stinner in branch 'master':
bpo-36854: Fix refleak in subinterpreter (GH-17331)
https://github.com/python/cpython/commit/310e2d25170a88ef03f6fd31efcc899fe062da2c
msg357269 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-22 11:36
> New changeset 310e2d25170a88ef03f6fd31efcc899fe062da2c by Victor Stinner in branch 'master':
> bpo-36854: Fix refleak in subinterpreter (GH-17331)

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).
msg357273 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-22 12:39
New changeset 84c36c152a2bdf98f9cc7ce0e1db98e1f442a05e by Victor Stinner in branch '3.8':
bpo-36854: Fix reference counter in PyInit__testcapi() (GH-17338)
https://github.com/python/cpython/commit/84c36c152a2bdf98f9cc7ce0e1db98e1f442a05e
msg357275 - (view) Author: miss-islington (miss-islington) Date: 2019-11-22 12:57
New changeset bff525566469021949910deec84e837306b79886 by Miss Islington (bot) in branch '3.7':
bpo-36854: Fix reference counter in PyInit__testcapi() (GH-17338)
https://github.com/python/cpython/commit/bff525566469021949910deec84e837306b79886
msg357276 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-22 13:00
I close again the issue ;-)
msg357328 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-11-22 22:21
Thanks so much for getting this done, Victor!

> I'm not fully happy with this solution

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?
msg357329 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-11-22 22:23
Did I mention that you're my hero? :)
msg357795 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-12-04 11:36
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 https://github.com/python/cpython/pull/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
msg357796 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-12-04 11:39
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!"
msg358355 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-12-13 21:27
On Wed, Dec 4, 2019 at 4:36 AM STINNER Victor <report@bugs.python.org> wrote:
> 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.

+1

> 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.

+1

> 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 :-)

Sounds good. :)

On Wed, Dec 4, 2019 at 4:39 AM STINNER Victor <report@bugs.python.org> wrote:
> 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 ;-)

Again, thanks for that!
History
Date User Action Args
2020-05-15 00:37:30vstinnersetcomponents: + Subinterpreters, - Interpreter Core
2019-12-13 21:27:21eric.snowsetmessages: + msg358355
2019-12-04 11:39:08vstinnersetmessages: + msg357796
2019-12-04 11:36:37vstinnersetmessages: + msg357795
2019-12-03 21:45:20pablogsalsetpull_requests: + pull_request16936
2019-11-22 22:23:33eric.snowsetmessages: + msg357329
2019-11-22 22:21:47eric.snowsetmessages: + msg357328
2019-11-22 13:00:22vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg357276

stage: patch review -> resolved
2019-11-22 12:57:02miss-islingtonsetnosy: + miss-islington
messages: + msg357275
2019-11-22 12:39:56miss-islingtonsetpull_requests: + pull_request16823
2019-11-22 12:39:54vstinnersetmessages: + msg357273
2019-11-22 11:36:32vstinnersetmessages: + msg357269
2019-11-22 11:30:13vstinnersetpull_requests: + pull_request16822
2019-11-22 09:58:03vstinnersetmessages: + msg357263
2019-11-21 23:13:49vstinnersetstage: resolved -> patch review
pull_requests: + pull_request16818
2019-11-21 11:47:38vstinnersetmessages: + msg357160
2019-11-21 10:17:00vstinnersetmessages: + msg357151
2019-11-21 10:15:44vstinnersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg357150
2019-11-20 11:27:55vstinnersetstatus: open -> closed
versions: + Python 3.9, - Python 3.8
messages: + msg357062

resolution: fixed
stage: patch review -> resolved
2019-11-20 11:25:55vstinnersetmessages: + msg357061
2019-11-20 11:03:57vstinnersetpull_requests: + pull_request16780
2019-11-20 10:48:22vstinnersetmessages: + msg357053
2019-11-20 10:23:35vstinnersetpull_requests: + pull_request16778
2019-11-20 10:17:21vstinnersetnosy: + vstinner
messages: + msg357048
2019-11-20 09:45:01vstinnersetpull_requests: + pull_request16772
2019-08-22 02:24:54phsilvasetnosy: + phsilva
2019-05-09 15:38:07eric.snowsetkeywords: + patch
stage: patch review
pull_requests: + pull_request13130
2019-05-08 17:00:25eric.snowcreate