GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||
GeezerVison is showing github.com — gh-116738: Inline append for local PyList by yoney · Pull Request #135196 · python/cpython Conversation
|
|
Can you add a sentence or two about the motivation for this change (performance?) to the PR description? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Updated the description, thank you! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm a little skeptical that the performance is ever going to matter (getgrall() is unlikely to be very big and if it is, it's always going to be horrendously slow) but the code is clear enough and the assert protects us against unintended sharing of the list, so why not.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| struct group *p; | ||
| // `d` is a local list; append items without a lock using | ||
| // _PyList_AppendTakeRef() within the loop. | ||
| assert(_PyObject_IsUniquelyReferenced(d)); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
d is not passed anywhere, isn't this obvious? we never added such assertions where it was obvious like this case
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I doubt that getgrall would produce so many entries to that performance of this would matter, using private API even in non core extension modules and non performance critical code seems like a bad idea to me so -1 from me. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@Yhg1s, @kumaraditya303: Thank you for the reviews. I completely agree that performance is very unlikely to matter here for @kumaraditya303 As far as I understand, you prefer to use the private API only when the performance wins can be clearly measured. @Yhg1s @kumaraditya303, I could close the PR and, in the future, make changes only when the performance benefits of using private APIs are clearly measurable. What do you think? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Yes, that sounds better |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yes, I think that's the right approach in this. (Your efforts are nonetheless appreciated, Alper :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.