Add a cross-interpreter-safe mechanism to indicate that an object may be destroyed.

[ericsnowcurrently]

BPO	33608
Nosy	@nascheme, @ncoghlan, @vstinner, @phsilva, @pmp-p, @markshannon, @ericsnowcurrently, @serhiy-storchaka, @soltysh, @1st1, @koobs, @zooba, @phmc, @emilyemorehouse, @jdahlin
PRs	[WIP] bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). #9334 bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). #11617 bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). #11617 bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). #11617 bpo-33608: Use _Py_AddPendingCall() in _PyCrossInterpreterData_Release(). #12024 bpo-33608: Simplify DISPATCH by hoisting eval_breaker ahead of time. #12062 Revert: bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). (GH-11617) #12159 bpo-33608: Fix the Py_atomic_* macros. #12240 bpo-33608: Simplify DISPATCH by hoisting eval_breaker ahead of time. #12243 bpo-33608: Make sure locks in the runtime are properly re-created.  #12245 bpo-33608: Deal with pending calls relative to runtime shutdown. #12246 bpo-33608: Minor cleanup related to pending calls. #12247 bpo-33608: Fix PyEval_InitThreads() warning #12346 bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). #12360 bpo-33608: make arm macros match x86 macros #12665 bpo-33608: Revert "Factor out a private, per-interpreter _Py_AddPendingCall()." #12806 bpo-33608: Normalize atomic macros so that they all expect an atomic struct #12877 bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). #13714 Revert "bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). (gh-13714)" #13780
Files	managers.patch take_gil.assert.patch fini_crash.c wrap_threadstate.diff

^{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 2020-04-08.21:58:41.738>
created_at = <Date 2018-05-22.19:34:41.672>
labels = ['expert-subinterpreters', '3.9']
title = 'Add a cross-interpreter-safe mechanism to indicate that an object may be destroyed.'
updated_at = <Date 2020-06-03.16:43:06.602>
user = 'https://github.com/ericsnowcurrently'

bugs.python.org fields:

activity = <Date 2020-06-03.16:43:06.602>
actor = 'vstinner'
assignee = 'eric.snow'
closed = True
closed_date = <Date 2020-04-08.21:58:41.738>
closer = 'vstinner'
components = ['Subinterpreters']
creation = <Date 2018-05-22.19:34:41.672>
creator = 'eric.snow'
dependencies = []
files = ['48333', '48334', '48731', '48732']
hgrepos = []
issue_num = 33608
keywords = ['patch']
message_count = 82.0
messages = ['317333', '317334', '317344', '317404', '325481', '325548', '336490', '336544', '336596', '336602', '336603', '336677', '336687', '336897', '336948', '336951', '336952', '336975', '337075', '337096', '337110', '337115', '337116', '337117', '337159', '337179', '337218', '337235', '337554', '337557', '337996', '338039', '340057', '340069', '340070', '340071', '340075', '340077', '340079', '340082', '340133', '340139', '340143', '340145', '340666', '340768', '342791', '342792', '344210', '344211', '344240', '344244', '344245', '344355', '344372', '344417', '344429', '344434', '344435', '344438', '344441', '344442', '344443', '344444', '344445', '344446', '357169', '357170', '357179', '358364', '358367', '358748', '364670', '365160', '365163', '365178', '366009', '366013', '366016', '366017', '366019', '366027']
nosy_count = 16.0
nosy_names = ['nascheme', 'ncoghlan', 'vstinner', 'phsilva', 'pmpp', 'Mark.Shannon', 'eric.snow', 'serhiy.storchaka', 'maciej.szulik', 'yselivanov', 'koobs', 'steve.dower', 'pconnell', 'emilyemorehouse', 'Johan Dahlin', 'shprotx']
pr_nums = ['9334', '11617', '11617', '11617', '12024', '12062', '12159', '12240', '12243', '12245', '12246', '12247', '12346', '12360', '12665', '12806', '12877', '13714', '13780']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue33608'
versions = ['Python 3.9']

[ericsnowcurrently]

In order to keep subinterpreters properly isolated, objects
from one interpreter should not be used in C-API calls in
another interpreter. That is relatively straight-forward
except in one case: indicating that the other interpreter
doesn't need the object to exist any more (similar to
PyBuffer_Release() but more general). I consider the
following solutions to be the most viable. Both make use
of recounts to protect cross-interpreter usage (with incref
before sharing).

1. manually switch interpreters (new private function)
   a. acquire the GIL
   b. if refcount > 1 then decref and release the GIL
   c. switch
   d. new thread (or re-use dedicated one)
   e. decref
   f. kill thread
   g. switch back
   h. release the GIL
2. change pending call mechanism (see Py_AddPendingCall) to
   per-interpreter instead of global (add "interp" arg to
   signature of new private C-API function)
   a. queue a function that decrefs the object
3. new cross-interpreter-specific private C-API function
   a. queue the object for decref (a la Py_AddPendingCall)
   in owning interpreter

I favor #2, since it is more generally applicable. #3 would
probably be built on #2 anyway. #1 is relatively inefficient.
With #2, Py_AddPendingCall() would become a simple wrapper
around the new private function.

[ericsnowcurrently]

As a lesser (IMHO) alternative, we could also modify Py_DECREF
to respect a new "shared" marker of some sort (perhaps relative
to bpo-33607), but that would probably still require one of the
refcount-based solutions (and add a branch to Py_DECREF).

[vstinner]

"That is relatively straight-forward except in one case: indicating that the other interpreter doesn't need the object to exist any more (similar to PyBuffer_Release() but more general)"

Why an interpreter would access an object from a different interpreter? Each interpreter should have its own object space, no?

[ncoghlan]

Adding a low level callback based mechanism to ask another interpreter to do work seems like a good way to go to me.

As an example of why that might be needed, consider the case of sharing a buffer exporting object with another subinterpreter: when the memoryview in the subinterpreter is destroyed, it needs to request that the buffer view be released in the source interpreter that actually owns the original object.

[nascheme]

I would suggest that sharing of objects between interpreters should be stamped out. Could we have some #ifdef debug checking that would warn or assert so this doesn't happen? I know currently we share a lot of objects. However, in the long term, that does not seem like the correct design. Instead, each interpreter should have its own objects and any passing or sharing should be tightly controlled.

[ericsnowcurrently]

@neil, we're definitely on the same page. In fact, in a world where subinterpreters do not share a GIL, we can't ever use an object in one interpreter that was created in another (due to thread safety on refcounts). The role of "tightly controlling" passing/sharing objects (for a very loose definition of "sharing") falls to the channels described in PEP-554. [1]

However, there are several circumstances where interpreters may collaborate that involves one holding a reference (but not using it) to an object owned by the other. For instance, see PyBuffer_Release(). [2] This issue is about addressing that situation safely. It is definitely not about safely using objects from other interpreters.

[1] The low-level implementation, including channels, already exists in Modules/_xxsubinterpretersmodule.c.
[2] https://docs.python.org/3/c-api/buffer.html#c.PyBuffer_Release

[ericsnowcurrently]

New changeset ef4ac96 by Eric Snow in branch 'master':
bpo-33608: Factor out a private, per-interpreter _Py_AddPendingCall(). (GH-11617)
ef4ac96

[ericsnowcurrently]

#55826 has introduces a slight performance regression which will need to be resolved. If I can't find the problem right away then I'll probably temporarily revert the commit.

See https://mail.python.org/pipermail/python-dev/2019-February/156451.html.

[ericsnowcurrently]

At least, it *might* be a performance regression. Here are the two commits I tried:

after: ef4ac96 (PR bpo-11617, from this issue)
before: 463572c (the one right before)

After building each (a normal build, not debug), I ran the micro-benchmark Raymond referred to ("./python Tools/scripts/var_access_benchmark.py") multiple times. There was enough variability between runs from the same commit that I'm uncertain at this point if there really is any performance regression. For the most part the results between the two commits are really close. Also, the results from the "after" commit fall within the variability I've seen between runs of the "before" commit.

I'm going to try with the "performance" suite (https://github.com/python/performance) to see if it shows any regression.

[ericsnowcurrently]

Here's what I did (more or less):

$ python3 -m pip install --user perf
$ python3 -m perf system tune
$ git clone git@github.com:python/performance.git
$ git clone git@github.com:python/cpython.git
$ cd cpython
$ git checkout ef4ac967e2f3a9a18330cc6abe14adb4bc3d0465
$ ./configure
$ make -j8
$ ./python ../performance/pyperformance run --fast --output speed.after
$ git checkout HEAD^
$ make -j8
$ ./python ../performance/pyperformance run --fast --output speed.before
$ ./python ../performance/pyperformance compare speed.before speed.after -O table

Here are my results:

speed.before
============

Performance version: 0.7.0
Report on Linux-4.15.0-45-generic-x86_64-with-glibc2.26
Number of logical CPUs: 4
Start date: 2019-02-25 20:39:44.151699
End date: 2019-02-25 20:48:04.817516

speed.after
===========

Performance version: 0.7.0
Report on Linux-4.15.0-45-generic-x86_64-with-glibc2.26
Number of logical CPUs: 4
Start date: 2019-02-25 20:29:56.610702
End date: 2019-02-25 20:38:11.667461

+-------------------------+--------------+-------------+--------------+-----------------------+
| Benchmark | speed.before | speed.after | Change | Significance |
+=========================+==============+=============+==============+=======================+
| 2to3 | 447 ms | 440 ms | 1.02x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| chaos | 155 ms | 156 ms | 1.01x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| crypto_pyaes | 154 ms | 152 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| deltablue | 10.7 ms | 10.5 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| django_template | 177 ms | 172 ms | 1.03x faster | Significant (t=3.66) |
+-------------------------+--------------+-------------+--------------+-----------------------+
| dulwich_log | 105 ms | 106 ms | 1.01x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| fannkuch | 572 ms | 573 ms | 1.00x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| float | 149 ms | 146 ms | 1.02x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| go | 351 ms | 349 ms | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| hexiom | 14.6 ms | 14.4 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| html5lib | 126 ms | 122 ms | 1.03x faster | Significant (t=3.46) |
+-------------------------+--------------+-------------+--------------+-----------------------+
| json_dumps | 17.6 ms | 17.2 ms | 1.02x faster | Significant (t=2.65) |
+-------------------------+--------------+-------------+--------------+-----------------------+
| json_loads | 36.4 us | 36.1 us | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| logging_format | 15.2 us | 14.9 us | 1.02x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| logging_silent | 292 ns | 296 ns | 1.01x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| logging_simple | 13.7 us | 13.4 us | 1.02x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| mako | 22.9 ms | 22.5 ms | 1.02x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| meteor_contest | 134 ms | 134 ms | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| nbody | 157 ms | 161 ms | 1.03x slower | Significant (t=-3.85) |
+-------------------------+--------------+-------------+--------------+-----------------------+
| nqueens | 134 ms | 132 ms | 1.02x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| pathlib | 30.1 ms | 31.0 ms | 1.03x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| pickle | 11.5 us | 11.6 us | 1.01x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| pickle_dict | 29.5 us | 30.5 us | 1.03x slower | Significant (t=-6.37) |
+-------------------------+--------------+-------------+--------------+-----------------------+
| pickle_list | 4.54 us | 4.58 us | 1.01x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| pickle_pure_python | 652 us | 651 us | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| pidigits | 212 ms | 215 ms | 1.02x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| python_startup | 11.6 ms | 11.5 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| python_startup_no_site | 8.07 ms | 7.95 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| raytrace | 729 ms | 722 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| regex_compile | 249 ms | 247 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| regex_dna | 203 ms | 204 ms | 1.00x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| regex_effbot | 3.55 ms | 3.55 ms | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| regex_v8 | 28.3 ms | 28.3 ms | 1.00x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| richards | 105 ms | 105 ms | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| scimark_fft | 429 ms | 436 ms | 1.01x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| scimark_lu | 238 ms | 237 ms | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| scimark_monte_carlo | 144 ms | 139 ms | 1.04x faster | Significant (t=3.61) |
+-------------------------+--------------+-------------+--------------+-----------------------+
| scimark_sor | 265 ms | 260 ms | 1.02x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| scimark_sparse_mat_mult | 5.41 ms | 5.25 ms | 1.03x faster | Significant (t=4.26) |
+-------------------------+--------------+-------------+--------------+-----------------------+
| spectral_norm | 174 ms | 173 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| sqlalchemy_declarative | 216 ms | 214 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| sqlalchemy_imperative | 41.6 ms | 41.2 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| sqlite_synth | 3.99 us | 3.91 us | 1.02x faster | Significant (t=2.49) |
+-------------------------+--------------+-------------+--------------+-----------------------+
| sympy_expand | 559 ms | 559 ms | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| sympy_integrate | 25.2 ms | 25.3 ms | 1.00x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| sympy_str | 261 ms | 260 ms | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| sympy_sum | 136 ms | 138 ms | 1.01x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| telco | 8.36 ms | 8.32 ms | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| tornado_http | 273 ms | 271 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| unpack_sequence | 58.8 ns | 60.0 ns | 1.02x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| unpickle | 21.5 us | 21.5 us | 1.00x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| unpickle_list | 5.60 us | 5.55 us | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| unpickle_pure_python | 497 us | 481 us | 1.03x faster | Significant (t=5.04) |
+-------------------------+--------------+-------------+--------------+-----------------------+
| xml_etree_generate | 141 ms | 140 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| xml_etree_iterparse | 131 ms | 133 ms | 1.01x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| xml_etree_parse | 186 ms | 187 ms | 1.00x slower | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+
| xml_etree_process | 115 ms | 113 ms | 1.01x faster | Not significant |
+-------------------------+--------------+-------------+--------------+-----------------------+

[ericsnowcurrently]

...so it doesn't appear that my PR introduces a performance regression.

[vstinner]

...so it doesn't appear that my PR introduces a performance regression.

IMHO there is no performance regression at all. Just noice in the result which doesn't come with std dev.