-
-
Notifications
You must be signed in to change notification settings - Fork 37.4k
gh-158592: Skip reallocation when shrinking a small list #158787
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
base: main
Are you sure you want to change the base?
Changes from all commits
bbad8e9
23ad656
5cd80db
89b411f
a4f3cff
dd855fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Skip the array reallocation when shrinking a small :class:`list`. Patch by | ||
| Donghee Na. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,6 +91,10 @@ ensure_shared_on_resize(PyListObject *self) | |
| #endif | ||
| } | ||
|
|
||
| #define LIST_SMALL_ALLOCATED 32 | ||
|
|
||
| Py_NO_INLINE static int py_list_resize(PyListObject *self, Py_ssize_t newsize); | ||
|
|
||
| /* Ensure ob_item has room for at least newsize elements, and set | ||
| * ob_size to newsize. If newsize > ob_size on entry, the content | ||
| * of the new slots at exit is undefined heap trash; it's the caller's | ||
|
|
@@ -100,21 +104,32 @@ ensure_shared_on_resize(PyListObject *self) | |
| * Note that self->ob_item may change, and even if newsize is less | ||
| * than ob_size on entry. | ||
| */ | ||
| static int | ||
| static inline Py_ALWAYS_INLINE int | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And why is this one |
||
| list_resize(PyListObject *self, Py_ssize_t newsize) | ||
| { | ||
| size_t new_allocated, target_bytes; | ||
| Py_ssize_t allocated = self->allocated; | ||
|
|
||
| /* 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. | ||
| gh-158602: do not shrink a small list, the realloc() cost is bigger | ||
| than the memory we get back. | ||
| */ | ||
| if (allocated >= newsize && newsize >= (allocated >> 1)) { | ||
| if (allocated >= newsize | ||
| && (newsize >= (allocated >> 1) || allocated <= LIST_SMALL_ALLOCATED)) | ||
| { | ||
| assert(self->ob_item != NULL || newsize == 0); | ||
| Py_SET_SIZE(self, newsize); | ||
| return 0; | ||
| } | ||
| return py_list_resize(self, newsize); | ||
| } | ||
|
|
||
| Py_NO_INLINE static int | ||
| py_list_resize(PyListObject *self, Py_ssize_t newsize) | ||
|
Comment on lines
+128
to
+129
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment explaining why we're forcing this to not be inlined? |
||
| { | ||
| size_t new_allocated, target_bytes; | ||
| Py_ssize_t allocated = self->allocated; | ||
|
|
||
| /* This over-allocates proportional to the list size, making room | ||
| * for additional growth. The over-allocation is mild, but is | ||
|
|
@@ -136,6 +151,8 @@ list_resize(PyListObject *self, Py_ssize_t newsize) | |
| if (newsize == 0) | ||
| new_allocated = 0; | ||
|
|
||
| assert(newsize > allocated || new_allocated < (size_t)allocated); | ||
|
|
||
| ensure_shared_on_resize(self); | ||
|
|
||
| #ifdef Py_GIL_DISABLED | ||
|
|
@@ -1148,7 +1165,7 @@ list_ass_item_lock_held(PyListObject *a, Py_ssize_t i, PyObject *v) | |
| for (Py_ssize_t idx = i; idx < size - 1; idx++) { | ||
| FT_ATOMIC_STORE_PTR_RELEASE(a->ob_item[idx], a->ob_item[idx + 1]); | ||
| } | ||
| Py_SET_SIZE(a, size - 1); | ||
| list_resize(a, size - 1); // NB: shrinking a list can't fail | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, you are doing the same thing I did in my PR. Please do not do this: to me, it does not seem very ethical. Keep exactly what the title says: the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @x42005e1f This is the reason why I added you as co-authored of this patch. But your initial proposal would be very difficult to accept because of performance issue. To supplement your patch, I spent most of time for the data analysis. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In that case, why not merge this PR without the fix first, and then mine, or vice versa? What is the problem? You do not even have a NEWS entry about the fix. I even mentioned a possible tuning of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do that if you want. But for the performance optimization, we need to understand how it worth to do for especially for this kind of performance regression issue. I am fine with merging this PR after your PR is merged. This is part of effort to pursuade your issue is worth to fix.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better communication for this kind of issue is that
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To me, the second option seems preferable, so I am requesting it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In anycase, we need to wait until other core devs like this approach. So please be patient.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imo. we should not merge a PR that we do not prefer and then merge a fix straight after. We could pull the changes here (or whatever changes we prefer in the end) into the first open branch though. |
||
| } | ||
| else { | ||
| FT_ATOMIC_STORE_PTR_RELEASE(a->ob_item[i], Py_NewRef(v)); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But large arrays are re-allocated! This PR solves a bug (feature?), which should be in the news entry.