gh-158592: Reallocate the list's array when deleting by index

[x42005e1f]

Fixes the linked issue with a simple one-line change, copy-pasted from list_pop_impl(). Ideally, the v == NULL branch of the list_ass_item_lock_held() function should be code-synchronized with the one mentioned, since the implementation of list_pop_impl() appears to be more efficient, but that is beyond the scope of this PR.

Performance comparison:

Details

$ ./python -m pyperf timeit -s 'obj = []' 'obj.append(None); del obj[-1]' --duplicate 1000
.....................
Mean +- std dev: 19.3 ns +- 0.0 ns
$ ./python -m pyperf timeit -s 'obj = [None]' 'obj.append(None); del obj[-1]' --duplicate 1000
.....................
Mean +- std dev: 19.3 ns +- 0.1 ns
$ ./python -m pyperf timeit -s 'obj = []' 'obj.append(None); del obj[-1]' --duplicate 1000
.....................
Mean +- std dev: 50.3 ns +- 0.3 ns
$ ./python -m pyperf timeit -s 'obj = [None]' 'obj.append(None); del obj[-1]' --duplicate 1000
.....................
Mean +- std dev: 27.1 ns +- 0.1 ns

Benchmark	before	after
0-1 items	19.3 ns	50.3 ns: 2.61x slower
1-2 items	19.3 ns	27.1 ns: 1.40x slower

The first case always reallocates the array, while the second case never reallocates the array (see #158602 (comment)). The results appear to be a performance regression, but it is important to understand what this change actually does. Namely, it simply restores the missed reallocation that other methods already perform (del list[slice], list.pop(), list.remove(), etc.)! If you are concerned about this, you should change the list_resize() implementation rather than making the collection non-shrinkable in terms of capacity.

I decided not to add a regression test because it would depend on the exact behavior of the list_resize() function (specifically, when it decides to change the capacity), which would require updating the test whenever its condition changed.

• Issue: del list[index] never reallocates the array #158592

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[ZeroIntensity]

If you are concerned about this, you should change the list_resize() implementation rather than making the collection non-shrinkable in terms of capacity.

There's not much to change in list_resize. This PR is just inherently going to be slower than what we currently have, because it's always going to be doing more.

Do you have benchmarks on the memory usage improvement? If all we're saving is a few bytes while being 160% slower, then I would be -1 on this.

[x42005e1f]

Do you have benchmarks on the memory usage improvement? If all we're saving is a few bytes while being 160% slower, then I would be -1 on this.

I do not understand the question. The issue is that this operation never (!) frees up memory. You can fill the list with 1_000 elements (+8 KB), or with 1_000_000 elements (+8 MB), and no del list[index] will free up memory. The point is not about "saving a few bytes" but rather about restoring the memory release mechanism in principle.

[x42005e1f]

Let me put it another way: if you think this is not worth changing, then why does the list shrink at all during other operations that remove items? Why do not we just remove the shrinking altogether, and thus speed up all similar methods (for example, list.pop(index), which I have mentioned repeatedly, is functionally equivalent to del list[index], with the sole exception that it returns the removed item)? Then no list would free up memory unless it died, and all methods would be very fast, because sooner or later the array's reallocating to a larger capacity would simply stop (since there would be one large enough that no further resizing would be needed).

[picnixz]

A question but:

If you are concerned about this, you should change the list_resize() implementation rather than making the collection non-shrinkable in terms of capacity.

Who is the "you"?

[ZeroIntensity]

Deleting an item from a list is common. Deleting many items at once using del is not. So far, I think the missing reallocation might just be a happy accident that improves performance.

I do not understand the question.

I meant by running our test suite or something like that, or just by running your microbenchmark.

[x42005e1f]

Who is the "you"?

A potential reader who is able to contribute to the repository. Excuse me if the tone of this sentence seems out of place (actually, I was just trying to point out the logic; though, there are already so many of my stalled issues in python/cpython that perhaps I am wasting my time on this…). I had my doubts about it too; editing it a couple of hours after posting seems wrong to me.

Deleting an item from a list is common. Deleting many items at once using del is not.

It is not necessary to remove many items at once. Surely there is someone who uses del list[index] as a frequent operation. For example, I use list.append()+list.pop() as an important pair of operations in my library, but I could just as easily use del list[-1] instead of the second one, since I do nothing with the item I receive. And then the lists would never free up memory, due to the issue.

I meant by running our test suite or something like that, or just by running your microbenchmark.

Again, I do not understand what you want me to measure. In the current implementation, the list capacity does not decrease when deleting by index. Why measure logic that is already clearly demonstrated in the issue using sys.getsizeof()?

[picnixz]

Btw, we do bypass the reallocation in list_resize() sometimes:

    /* Bypass realloc() when a previous overallocation is large enough
       to accommodate the newsize.  If the newsize falls lower than half
       the allocated size, then proceed with the realloc() to shrink the list.
    */
    if (allocated >= newsize && newsize >= (allocated >> 1)) {
        assert(self->ob_item != NULL || newsize == 0);
        Py_SET_SIZE(self, newsize);
        return 0;
    }

Depending on the allocated value, you might be hitting different paths. I wonder if your empty case is some magical threshold (it wouldbe good to know what happens with lists of various sizes as all your tests are only for lists of size 0 or 1, and I don't know if this happy path is taken or not and which path is actually taken).

I also believe that the .append() call should not be part of the timings. Only the del statement must be benchmarked.

---

What's the effect of that on non-free threaded builds (or on FT builds, I don't know which one you used).

---

So the call to Py_SET_SIZE would still be there if we allocated a large list at the beginning already.

Again, I do not understand what you want me to measure

After your PR, is the sys.getsizeof() call you showed on the issue behaving properly?

[ZeroIntensity]

Surely there is someone who uses del list[index] as a frequent operation. For example, I use list.append()+list.pop() as an important pair of operations in my library, but I could just as easily use del list[-1] instead of the second one, since I do nothing with the item I receive. And then the lists would never free up memory, due to the issue.

Hm? They wouldn't need to free things up. If you're frequently deleting and appending, then not resizing on delete is the more performant option.

Generally speaking, I'm more for speed efficiency than for memory efficiency (Python users rarely care about saving a little memory, and those who do will be using the array module or NumPy anyway), but at the end of the day, this is basically just bikeshedding, one way or the other. I'm not going to put effort into reviewing this, but if another core dev feels that this is a worthwhile change, don't consider me a blocker.

[x42005e1f]

Depending on the allocated value, you might be hitting different paths. I wonder if your empty case is some magical threshold (it wouldbe good to know what happens with lists of various sizes as all your tests are only for lists of size 0 or 1, and I don't know if this happy path is taken or not and which path is actually taken).

The first case:

1. allocated == 0
2. append; allocated == 0 && newsize == 1 => reallocate
3. allocated == 4
4. delete; allocated == 4 && newsize == 0 => reallocate
5. Go to 1.

The second case:

1. allocated == 1
2. append; allocated == 1 && newsize == 2 => reallocate
3. allocated == 8
4. delete; allocated == 8 && newsize == 1 => reallocate
5. allocated == 4
6. append; allocated == 4 && newsize == 2 => skip (2 >= 2)
7. allocated == 4
8. delete; allocated == 4 && newsize == 1 => reallocate
9. allocated == 4
10. Go to 6.

I was wrong about the second case, since it actually reallocates the array (after the PR), but to the exact same capacity (perhaps an oversight in the implementation?).

See new_allocated.

I also believe that the .append() call should not be part of the timings. Only the del statement must be benchmarked.

I agree, but I am not sure how to do that with pyperf. So I literally did exactly what you asked for in #158592 (comment).

What's the effect of that on non-free threaded builds (or on FT builds, I don't know which one you used).

In theory, its performance will be close to that of list.pop(index), but it will be slower because the implementation lacks the fast path of the ptr_wise_atomic_memove() function (and the fast single-element list handling). As for thread-safety, I should note that the lock_held suffix in list_ass_item_lock_held hints at this.

Oh, yes, I used a non-free-threaded build. If it had been otherwise, I would have mentioned it. But since the issue is not bound to free threading, I assumed it was obvious.

After your PR, is the sys.getsizeof() call you showed on the issue behaving properly?

Yes.

[x42005e1f]

Hm? They wouldn't need to free things up. If you're frequently deleting and appending, then not resizing on delete is the more performant option.

The peak size is not the same as the average size, I should note. From a memory perspective, not freeing up memory is much worse, especially if there are many lists.

Python users rarely care about saving a little memory, and those who do will be using the array module or NumPy anyway

I disagree. Lists of arbitrary objects are not what NumPy is about.

[x42005e1f]

I was wrong about the second case, since it actually reallocates the array (after the PR), but to the exact same capacity (perhaps an oversight in the implementation?).

For 3-4 items:

1. allocated == 4
2. append; allocated == 4 && newsize == 4 => skip (4 >= 2)
3. allocated == 4
4. delete; allocated == 4 && newsize == 3 => skip (3 >= 2)
5. Go to 1.

$ ./python -m pyperf timeit -s 'obj = [None, None, None]' 'obj.append(None); del obj[-1]' --duplicate 1000
.....................
Mean +- std dev: 21.4 ns +- 0.1 ns

(Before: 19.3, so +2.1 ns)

[x42005e1f]

After your PR, is the sys.getsizeof() call you showed on the issue behaving properly?

Yes.

To be clear, you can simply compare list_ass_item_lock_held() (the v == NULL branch) with list_pop_impl() (also synchronized; just mentioning it just in case). They do almost the same thing (with the exception mentioned above regarding the return value).

---

Well, it seems a bit off that we are having a discussion here about whether the fix is actually needed, which essentially continues the discussion from the issue. This does not really feel like a review, but… oh well.

If we imagine that we are from the Ministry of Silly Walks… (just a joke, in support of how I myself was mistaken about the behavior of list_resize(), incorrectly assuming that a constant capacity meant there was no reallocation)

[ZeroIntensity]

We're just being cautious; "2.61x slower" sets off alarm bells for a maintainer.