Repository navigation
GH-127058: Make PySequence_Tuple safer and probably faster. - #127758
markshannon merged 4 commits into
Conversation
Sorry, something went wrong.
| Fail: | ||
| Py_XDECREF(result); | ||
| PyObject *res = _PyList_AsTupleAndClear(list); | ||
| Py_DECREF(list); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
Did you run refleak tests? |
Sorry, something went wrong.
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
|
🤖 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. |
Sorry, something went wrong.
| self->ob_item = NULL; | ||
| Py_SET_SIZE(self, 0); | ||
| ret = _PyTuple_FromArraySteal(items, size); | ||
| free_list_items(items, false); |
There was a problem hiding this comment.
If _PyTuple_FromArraySteal, do we still want to free the list items?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes. _PyTuple_FromArraySteal steals the references, but doesn't do anything with the memory
Sorry, something went wrong.
There was a problem hiding this comment.
Typically it is passed a stack-allocated array, so it can't free it.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, I meant if _PyTuple_FromArraySteal fails.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes. _PyTuple_FromArraySteal consumes the all references regardless of whether it succeeds or fails.
It would be a pain to use otherwise.
Sorry, something went wrong.
…ython#127758) * Use a small buffer, then list when constructing a tuple from an arbitrary sequence.
|
note that this pull request modifies the behavior of the following code import time
class A:
def __len__(self):
raise NotImplementedError("infinite sequence")
def __iter__(self):
while True:
time.sleep(1)
yield 0
tuple(A())before this change, the above raises an error. Now it enters an infinite loop. by itself it isn't a problem because the fact that tuple() calls __len__ is an internal implementation detail and can be changed any time, but there isn't any other way for a class to declare itself as not convertible to list/tuple because it is infinite. There is no magic method __list__/__tuple__/__sequence__. the same thing can be said about tuple(range(10**17))—before the change, immediate MemoryError; after it, consumes an increasing amount of memory before the process gets killed (don't try to run this). |
Sorry, something went wrong.
Instead of allocating a tuple, then running arbitrary code with an incomplete tuple, this PR:
Since the vast majority of tuples are small, PySequence_Tuple now only does one allocation and no resizing in the common case.