Skip to content

gh-111623 Serialize tuples with sub interpreters#111628

Merged
ericsnowcurrently merged 34 commits intopython:mainfrom
tonybaloney:serialize_tuples
Nov 7, 2023
Merged

gh-111623 Serialize tuples with sub interpreters#111628
ericsnowcurrently merged 34 commits intopython:mainfrom
tonybaloney:serialize_tuples

Conversation

@tonybaloney
Copy link
Copy Markdown
Contributor

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

@bedevere-app bedevere-app bot mentioned this pull request Nov 2, 2023
Closed
@tonybaloney
Copy link
Copy Markdown
Contributor Author

tonybaloney commented Nov 2, 2023

Requesting a review from @ericsnowcurrently

tonybaloney
tonybaloney commented Nov 2, 2023
View reviewed changes
sobolevn
sobolevn reviewed Nov 2, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tonybaloney
Copy link
Copy Markdown
Contributor Author

tonybaloney commented Nov 2, 2023

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.

eendebakpt
eendebakpt reviewed Nov 2, 2023
View reviewed changes
ericsnowcurrently
ericsnowcurrently requested changes Nov 3, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
for s in non_shareables:
value = tuple([0, 1., s])
with self.subTest(repr(value)):
# XXX Assert the NotShareableError when it is exported
Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll work on that.

@bedevere-app
Copy link
Copy Markdown

bedevere-app bot commented Nov 3, 2023

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.

@tonybaloney
Copy link
Copy Markdown
Contributor Author

tonybaloney commented Nov 4, 2023

I have made the requested changes; please review again

@tonybaloney
Copy link
Copy Markdown
Contributor Author

tonybaloney commented Nov 4, 2023

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
Copy link
Copy Markdown

TeamSpen210 commented Nov 4, 2023

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
Copy link
Copy Markdown
Contributor Author

tonybaloney commented Nov 5, 2023

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

ericsnowcurrently
ericsnowcurrently requested changes Nov 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looking good. I only have a few comments.

@ericsnowcurrently
Copy link
Copy Markdown
Member

ericsnowcurrently commented Nov 6, 2023

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.

@tonybaloney
Copy link
Copy Markdown
Contributor Author

tonybaloney commented Nov 6, 2023 

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
Copy link
Copy Markdown
Contributor Author

tonybaloney commented Nov 6, 2023

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?

ericsnowcurrently
ericsnowcurrently approved these changes Nov 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ericsnowcurrently
Copy link
Copy Markdown
Member

ericsnowcurrently commented Nov 7, 2023

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

@ericsnowcurrently ericsnowcurrently merged commit 178861b into python:main Nov 7, 2023
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Nov 7, 2023

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

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

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (178861b) or if it is a false positive.
  5. 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
Copy link
Copy Markdown
Member

ericsnowcurrently commented Nov 7, 2023

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

hugovk pushed a commit to hugovk/cpython that referenced this pull request Nov 8, 2023
@tonybaloney @hugovk
pythongh-111623: Add Support for Cross-interpreter tuples (pythongh-1…
32be960 
…11628)
@tonybaloney 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
@tonybaloney @aisk
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
@tonybaloney @Glyphack
pythongh-111623: Add Support for Cross-interpreter tuples (pythongh-1…
944c0fa 
…11628)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eendebakpt eendebakpt eendebakpt left review comments

@sobolevn sobolevn sobolevn left review comments

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable cross-interpreter sharing of tuples

6 participants

@tonybaloney @TeamSpen210 @ericsnowcurrently @bedevere-bot @eendebakpt @sobolevn