gh-111623 Serialize tuples with sub interpreters #111628

tonybaloney · 2023-11-02T03:45:34Z

Closes #111623

Issue: Enable cross-interpreter sharing of tuples #111623

tonybaloney · 2023-11-02T04:38:36Z

Requesting a review from @ericsnowcurrently

Python/crossinterp.c

sobolevn

Question: tuples can contain mutable objects and objects that cannot be shared. What will happen in this case?

tonybaloney · 2023-11-02T22:06:58Z

Question: tuples can contain mutable objects and objects that cannot be shared. What will happen in this case?

It will raise a NotShareableError, I'll add a test for that scenario.

Lib/test/test_interpreters.py

Misc/NEWS.d/next/Core and Builtins/2023-11-02-15-00-57.gh-issue-111623.BZxYc8.rst

Python/crossinterp.c

…o serialize_tuples

…e-111623.BZxYc8.rst Co-authored-by: Pieter Eendebak <[email protected]>

Co-authored-by: Pieter Eendebak <[email protected]>

ericsnowcurrently

Overall I think you've got this right. I just have a few minor comments, and a possible small adjustment to how you are doing allocation.

I'm also pretty sure you have a number of memory leaks here. Be sure to check by running ./python -m test test__xxsubinterpreters -R 3:3. I've left a number of recommendations that should clean up the leaks.

Lib/test/test__xxsubinterpreters.py

ericsnowcurrently · 2023-11-03T16:15:23Z

Lib/test/test__xxsubinterpreters.py

+        for s in non_shareables:
+            value = tuple([0, 1., s])
+            with self.subTest(repr(value)):
+                # XXX Assert the NotShareableError when it is exported


I'll work on that.

Lib/test/test__xxsubinterpreters.py

Python/crossinterp.c

bedevere-app · 2023-11-03T17:40:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-authored-by: Eric Snow <[email protected]>

tonybaloney · 2023-11-04T08:10:09Z

I have made the requested changes; please review again

tonybaloney · 2023-11-04T08:14:15Z

and a possible small adjustment to how you are doing allocation.

I've lost this feedback, was it to change the structure to be closer to PyTuple's PyObject*[1] type? PyTuple has a custom allocation algorithm IIRC, so I'm not 100% sure how to correctly allocate those arrays correctly

TeamSpen210 · 2023-11-04T09:13:06Z

Should recursion limit checks be added? It's unlikely, but I think it is possible to create a cyclically recursive tuple, at least via the C API.

tonybaloney · 2023-11-05T02:19:30Z

Should recursion limit checks be added? It's unlikely, but I think it is possible to create a cyclically recursive tuple, at least via the C API.

It's not possible in Python, but it wouldn't hurt incase there were some other container type added to this module in future

…o serialize_tuples

ericsnowcurrently

Mostly looking good. I only have a few comments.

Python/crossinterp.c

ericsnowcurrently · 2023-11-06T19:04:35Z

and a possible small adjustment to how you are doing allocation.

I've lost this feedback, was it to change the structure to be closer to PyTuple's PyObject*[1] type? PyTuple has a custom allocation algorithm IIRC, so I'm not 100% sure how to correctly allocate those arrays correctly

Don't worry about this.

Co-authored-by: Eric Snow <[email protected]>

tonybaloney · 2023-11-06T21:05:00Z

test_tuples_containing_non_shareable_types (test.test__xxsubinterpreters.ShareableTypeTests.test_tuples_containing_non_shareable_types) ... Debug memory block at address p=0x560909c1f9c0: API '�'
    9275669246729650176 bytes originally requested
    The 7 pad bytes at p-7 are not all FORBIDDENBYTE (0xfd):
        at p-7: 0x00 *** OUCH
        at p-6: 0x94 *** OUCH
        at p-5: 0x09 *** OUCH
        at p-4: 0x09 *** OUCH
        at p-3: 0x56 *** OUCH
        at p-2: 0x00 *** OUCH
        at p-1: 0x00 *** OUCH
    Because memory is corrupted at the start, the count of bytes requested
       may be bogus, and checking the trailing pad bytes may segfault.
    The 8 pad bytes at tail=0x80ba22121317f9c0 are Fatal Python error: Segmentation fault

Current thread 0x00007fb4f4cb0280 (most recent call first):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test__xxsubinterpreters.py", line 218 in test_tuples_containing_non_shareable_types
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 589 in _callTestMethod
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 636 in run
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 692 in __call__
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 122 in run
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 84 in __call__
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 122 in run
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 84 in __call__
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/runner.py", line 240 in run
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/single.py", line 58 in _run_suite
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/single.py", line 38 in run_unittest
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/single.py", line 133 in test_func
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/single.py", line 89 in regrtest_runner
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/single.py", line 136 in _load_run_test
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/single.py", line 179 in _runtest_env_changed_exc
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/single.py", line 279 in _runtest
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/single.py", line 307 in run_single_test
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/worker.py", line 89 in worker_process
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/worker.py", line 112 in main
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/worker.py", line 116 in <module>
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/runpy.py", line 88 in _run_code
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/runpy.py", line 198 in _run_module_as_main

Extension modules: _testcapi, _testinternalcapi, _xxsubinterpreters (total: 3)

tonybaloney · 2023-11-06T21:31:54Z

Crashing in the negative case because of the test function:

if (_PyObject_GetCrossInterpreterData(obj, data) != 0) {
        _PyCrossInterpreterData_Free(data);
        return NULL;
    }

This is calling PyMem_RawFree because ->free is only set to our custom function at the end of the success cases (it isn't set in the error case)

* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007ff805b641e2 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007ff805b9bee6 libsystem_pthread.dylib`pthread_kill + 263
    frame #2: 0x00007ff805ac2b45 libsystem_c.dylib`abort + 123
    frame #3: 0x000000010025e24d python.exe`fatal_error_exit(status=<unavailable>) at pylifecycle.c:2773:9 [opt]
    frame #4: 0x000000010025e10d python.exe`fatal_error(fd=2, header=0, prefix=0x0000000000000000, msg=0x0000000000000000, status=-1) at pylifecycle.c:2954:5 [opt]
    frame #5: 0x000000010025e23b python.exe`_Py_FatalErrorFormat(func="_PyMem_DebugRawFree", format="bad ID: Allocated using API '%c', verified using API '%c'") at pylifecycle.c:3000:5 [opt]
    frame #6: 0x000000010010fd92 python.exe`_PyMem_DebugCheckAddress(func="_PyMem_DebugRawFree", api=<unavailable>, p=0x0000000100a4e570) at obmalloc.c:2497:9 [opt]
    frame #7: 0x000000010010fc5b python.exe`_PyMem_DebugRawFree(ctx=0x000000010059a970, p=0x0000000100a4e570) at obmalloc.c:2329:5 [opt]
    frame #8: 0x000000010010ecfb python.exe`PyMem_RawFree(ptr=<unavailable>) at obmalloc.c:818:5 [opt]
    frame #9: 0x000000010021eede python.exe`_PyCrossInterpreterData_Clear [inlined] _xidata_clear(data=0x0000000100a3dfa0) at crossinterp.c:122:13 [opt]
    frame #10: 0x000000010021eecb python.exe`_PyCrossInterpreterData_Clear(interp=<unavailable>, data=0x0000000100a3dfa0) at crossinterp.c:179:5 [opt]
    frame #11: 0x000000010021ee89 python.exe`_PyCrossInterpreterData_Free(xid=0x0000000100a3dfa0) at crossinterp.c:63:5 [opt]
    frame #12: 0x000000010407bf85 _testinternalcapi.cpython-313d-darwin.so`get_crossinterp_data(self=<unavailable>, args=<unavailable>) at _testinternalcapi.c:0:9 [opt]

I could move data->free = _tuple_shared_free; earlier in the function so it gets set when one of the subsequent errors happens, but this doesn't make much sense if the API assumes that the caller will run the free function if there is an error case?

…reed memory space

ericsnowcurrently

LGTM

ericsnowcurrently · 2023-11-07T00:55:49Z

I'll merge this first thing (my) tomorrow.

bedevere-bot · 2023-11-07T18:14:35Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD 3.x has failed when building commit 178861b.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1223/builds/696) and take a look at the build logs.
Check if the failure is related to this commit (178861b) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1223/builds/696

Failed tests:

test_interpreters

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "<frozen getpath>", line 352, in <module>
ValueError: embedded null byte
Warning -- Uncaught thread exception: RuntimeError
Exception in thread Thread-119 (task):
RuntimeError: error evaluating path


Traceback (most recent call last):
  File "/buildbot/buildarea/3.x.ware-freebsd/build/Lib/threading.py", line 1083, in _bootstrap_inner
    self.run()
  File "/buildbot/buildarea/3.x.ware-freebsd/build/Lib/threading.py", line 1020, in run
    self._target(*self._args, **self._kwargs)
  File "/buildbot/buildarea/3.x.ware-freebsd/build/Lib/test/test_interpreters.py", line 595, in task
    interp = interpreters.create()
             ^^^^^^^^^^^^^^^^^^^^^
  File "/buildbot/buildarea/3.x.ware-freebsd/build/Lib/test/support/interpreters.py", line 26, in create
    id = _interpreters.create(isolated=isolated)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
RuntimeError: interpreter creation failed
k

ericsnowcurrently · 2023-11-07T18:48:58Z

The buildbot failure appears to be another case of gh-109700.

…11628)

tonybaloney added 2 commits November 2, 2023 14:37

Initial design for shareable tuples

6eb3562

Add memory error

d3a5cfc

bedevere-app bot added the awaiting review label Nov 2, 2023

bedevere-app bot mentioned this pull request Nov 2, 2023

Enable cross-interpreter sharing of tuples #111623

Closed

tonybaloney added 3 commits November 2, 2023 14:58

Add a check for XID allocation failures

439b652

Add blurb

2d8602d

tweaks for pep7

3711bb5

tonybaloney commented Nov 2, 2023

View reviewed changes

Python/crossinterp.c Show resolved Hide resolved

sobolevn reviewed Nov 2, 2023

View reviewed changes

corona10 requested a review from ericsnowcurrently November 2, 2023 12:37

tonybaloney added 3 commits November 3, 2023 09:27

Add tests for non shareable tuples

7072d36

Merge branch 'main' into serialize_tuples

448d017

fix patchcheck

d6f386f

eendebakpt reviewed Nov 2, 2023

View reviewed changes

Lib/test/test_interpreters.py Show resolved Hide resolved

Misc/NEWS.d/next/Core and Builtins/2023-11-02-15-00-57.gh-issue-111623.BZxYc8.rst Outdated Show resolved Hide resolved

Python/crossinterp.c Show resolved Hide resolved

tonybaloney and others added 4 commits November 3, 2023 10:27

Merge branch 'serialize_tuples' of github.com:tonybaloney/cpython int…

fa958b3

…o serialize_tuples

Update Misc/NEWS.d/next/Core and Builtins/2023-11-02-15-00-57.gh-issu…

08cdfcb

…e-111623.BZxYc8.rst Co-authored-by: Pieter Eendebak <[email protected]>

Update Lib/test/test_interpreters.py

654689e

Co-authored-by: Pieter Eendebak <[email protected]>

Add two fault cases

994fab0

ericsnowcurrently requested changes Nov 3, 2023

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Nov 3, 2023

tonybaloney and others added 6 commits November 4, 2023 11:14

Update test__xxsubinterpreters.py

302dc8d

Co-authored-by: Eric Snow <[email protected]>

Update test__xxsubinterpreters.py

86c1494

Co-authored-by: Eric Snow <[email protected]>

Update test__xxsubinterpreters.py

3b30b77

Co-authored-by: Eric Snow <[email protected]>

Update test__xxsubinterpreters.py

9abda96

Co-authored-by: Eric Snow <[email protected]>

Update test__xxsubinterpreters.py

82f4836

Co-authored-by: Eric Snow <[email protected]>

Update test_interpreters.py

0fc4eab

Co-authored-by: Eric Snow <[email protected]>

tonybaloney and others added 2 commits November 4, 2023 11:18

Update crossinterp.c

063c8f7

Co-authored-by: Eric Snow <[email protected]>

Update the previous two blurbs to match this one

0bd3b88

Merge branch 'main' into serialize_tuples

e40381c

Add a recursion check

97f281c

Merge branch 'serialize_tuples' of github.com:tonybaloney/cpython int…

3b96658

…o serialize_tuples

ericsnowcurrently requested changes Nov 6, 2023

View reviewed changes

Python/crossinterp.c Outdated Show resolved Hide resolved

Python/crossinterp.c Outdated Show resolved Hide resolved

Python/crossinterp.c Outdated Show resolved Hide resolved

Python/crossinterp.c Outdated Show resolved Hide resolved

Python/crossinterp.c Outdated Show resolved Hide resolved

tonybaloney and others added 3 commits November 7, 2023 07:46

Update Python/crossinterp.c

f4ee75f

Co-authored-by: Eric Snow <[email protected]>

Update Python/crossinterp.c

1e1141e

Co-authored-by: Eric Snow <[email protected]>

Fix parens mismatch

1ea81a3

Refactor to avoid the failure case running PyMem_RawFree on already f…

a18939a

…reed memory space

ericsnowcurrently approved these changes Nov 6, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Nov 6, 2023

Merge branch 'main' into serialize_tuples

b986e3d

ericsnowcurrently merged commit 178861b into python:main Nov 7, 2023

bedevere-app bot removed the awaiting merge label Nov 7, 2023

hugovk pushed a commit to hugovk/cpython that referenced this pull request Nov 8, 2023

pythongh-111623: Add Support for Cross-interpreter tuples (pythongh-1…

32be960

…11628)

tonybaloney deleted the serialize_tuples branch November 17, 2023 19:52

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-111623: Add Support for Cross-interpreter tuples (pythongh-1…

1972f02

…11628)

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-111623: Add Support for Cross-interpreter tuples (pythongh-1…

944c0fa

…11628)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-111623 Serialize tuples with sub interpreters #111628

gh-111623 Serialize tuples with sub interpreters #111628

tonybaloney commented Nov 2, 2023 •

edited by bedevere-app bot

Loading

tonybaloney commented Nov 2, 2023

sobolevn left a comment

tonybaloney commented Nov 2, 2023

ericsnowcurrently left a comment

ericsnowcurrently Nov 3, 2023

bedevere-app bot commented Nov 3, 2023

tonybaloney commented Nov 4, 2023

tonybaloney commented Nov 4, 2023

TeamSpen210 commented Nov 4, 2023

tonybaloney commented Nov 5, 2023

ericsnowcurrently left a comment

ericsnowcurrently commented Nov 6, 2023

tonybaloney commented Nov 6, 2023

tonybaloney commented Nov 6, 2023

ericsnowcurrently left a comment

ericsnowcurrently commented Nov 7, 2023

bedevere-bot commented Nov 7, 2023

ericsnowcurrently commented Nov 7, 2023

gh-111623 Serialize tuples with sub interpreters #111628

gh-111623 Serialize tuples with sub interpreters #111628

Conversation

tonybaloney commented Nov 2, 2023 • edited by bedevere-app bot Loading

tonybaloney commented Nov 2, 2023

sobolevn left a comment

Choose a reason for hiding this comment

tonybaloney commented Nov 2, 2023

ericsnowcurrently left a comment

Choose a reason for hiding this comment

ericsnowcurrently Nov 3, 2023

Choose a reason for hiding this comment

bedevere-app bot commented Nov 3, 2023

tonybaloney commented Nov 4, 2023

tonybaloney commented Nov 4, 2023

TeamSpen210 commented Nov 4, 2023

tonybaloney commented Nov 5, 2023

ericsnowcurrently left a comment

Choose a reason for hiding this comment

ericsnowcurrently commented Nov 6, 2023

tonybaloney commented Nov 6, 2023

tonybaloney commented Nov 6, 2023

ericsnowcurrently left a comment

Choose a reason for hiding this comment

ericsnowcurrently commented Nov 7, 2023

bedevere-bot commented Nov 7, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

ericsnowcurrently commented Nov 7, 2023

tonybaloney commented Nov 2, 2023 •

edited by bedevere-app bot

Loading