gh-116738: Inline append for local PyList

[yoney]

Append items to the local list in a loop without a lock.

Appending to the list without a lock can improve performance, but testing grp.getgrall() produced noisy results, making it difficult to determine any improvement. Running a micro benchmark with a function that adds over 100K items to the list using _PyList_AppendTakeRef() instead of PyList_Append() showed about a 35% performance improvement.

cc: @mpage @colesbury

• Issue: Audit all built-in modules for thread safety #116738

[mpage]

Can you add a sentence or two about the motivation for this change (performance?) to the PR description?

[yoney]

Can you add a sentence or two about the motivation for this change (performance?) to the PR description?

Updated the description, thank you!

[ZeroIntensity]

LGTM.

[Yhg1s]

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.

[kumaraditya303]

d is not passed anywhere, isn't this obvious? we never added such assertions where it was obvious like this case

[kumaraditya303]

Appending to the list without a lock can improve performance, but testing grp.getgrall() produced noisy results, making it difficult to determine any improvement. Running a micro benchmark with a function that adds over 100K items to the list using _PyList_AppendTakeRef() instead of PyList_Append() showed about a 35% performance improvement.

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.

[yoney]

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)

I doubt that getgrall would produce so many entries to that performance of this would matter

@Yhg1s, @kumaraditya303: Thank you for the reviews. I completely agree that performance is very unlikely to matter here for getgrall(). I created this PR while working on grp module FT changes, and I want to gather feedback to determine exactly when we should use the private API.

@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?

[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?

Yes, that sounds better

[Yhg1s]

Yes, I think that's the right approach in this. (Your efforts are nonetheless appreciated, Alper :)