Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-127058: Make PySequence_Tuple safer and probably faster. #127758

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Dec 9, 2024

Instead of allocating a tuple, then running arbitrary code with an incomplete tuple, this PR:

  • Allocates a small fixed sized array, fills it then creates the tuple atomically.
  • If the buffer is too small, then a list is created. The tuple is created atomically from that list.
  • Adds the internal _PyList_AsTupleAndClear function which creates a tuple from a list while clearing the list. This avoids unnecessary reference count manipulation.

Since the vast majority of tuples are small, PySequence_Tuple now only does one allocation and no resizing in the common case.

@markshannon markshannon changed the title 127058: Make PySequence_Tuple safer and probably faster. GH-127058: Make PySequence_Tuple safer and probably faster. Dec 9, 2024
Objects/abstract.c Outdated Show resolved Hide resolved
Fail:
Py_XDECREF(result);
PyObject *res = _PyList_AsTupleAndClear(list);
Py_DECREF(list);
Copy link
Member

Choose a reason for hiding this comment

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

I think the name _PyList_AsTupleAndClear might be confusing, because if you called Py_CLEAR on the list you wouldn't need to decref it.

Copy link
Member Author

@markshannon markshannon Dec 11, 2024

Choose a reason for hiding this comment

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

But if you called PyList_Clear on it, you would need to Py_DECREF it.
It is the list being cleared, not the variable holding a reference to it.

@iritkatriel
Copy link
Member

Did you run refleak tests?

Co-authored-by: Irit Katriel <[email protected]>
@markshannon markshannon added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Dec 11, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 8badf7b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Dec 11, 2024
self->ob_item = NULL;
Py_SET_SIZE(self, 0);
ret = _PyTuple_FromArraySteal(items, size);
free_list_items(items, false);
Copy link
Member

Choose a reason for hiding this comment

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

If _PyTuple_FromArraySteal, do we still want to free the list items?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. _PyTuple_FromArraySteal steals the references, but doesn't do anything with the memory

Copy link
Member Author

Choose a reason for hiding this comment

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

Typically it is passed a stack-allocated array, so it can't free it.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant if _PyTuple_FromArraySteal fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. _PyTuple_FromArraySteal consumes the all references regardless of whether it succeeds or fails.
It would be a pain to use otherwise.

@markshannon markshannon merged commit 5a23994 into python:main Dec 11, 2024
55 checks passed
@markshannon markshannon deleted the safer-sequence-tuple branch December 11, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants