GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-116738: Inline append for local PyList by yoney · Pull Request #135196 · python/cpython

Conversation

@yoney

@yoney yoney commented Jun 6, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@yoney
pythongh-116738: Inline append for local PyList
70ba3ab
@bedevere-app bedevere-app Bot mentioned this pull request Jun 6, 2025
Audit all built-in modules for thread safety #116738
Open
@yoney
yoney marked this pull request as ready for review June 6, 2025 00:22
@bedevere-app bedevere-app Bot added the awaiting review label Jun 6, 2025
ZeroIntensity
ZeroIntensity reviewed Jun 6, 2025
View reviewed changes
Comment thread Modules/grpmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ZeroIntensity ZeroIntensity added skip news topic-free-threading labels Jun 6, 2025
@mpage
mpage requested review from Yhg1s, colesbury and mpage June 6, 2025 16:49
@mpage

mpage commented Jun 6, 2025

Copy link
Copy Markdown
Contributor

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

yoney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@yoney
pythongh-116738: Assert list is uniquely referenced
d47b85c
@yoney

yoney commented Jun 10, 2025

Copy link
Copy Markdown
Contributor Author

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

Updated the description, thank you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity
ZeroIntensity approved these changes Jun 10, 2025
View reviewed changes

@ZeroIntensity ZeroIntensity left a comment

Copy link
Copy Markdown
Member

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.

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Modules/grpmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Jun 10, 2025
@yoney
pythongh-116738: Let _PyList_AppendTakeRef() steal the reference
cf5d8ec
ZeroIntensity
ZeroIntensity reviewed Jun 10, 2025
View reviewed changes
Comment thread Modules/grpmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Yhg1s
Yhg1s approved these changes Jun 10, 2025
View reviewed changes

@Yhg1s Yhg1s left a comment

Copy link
Copy Markdown
Member

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

yoney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jun 10, 2025
@mpage
Merge branch 'main' into ft_grpmodule
9db2605
kumaraditya303
kumaraditya303 reviewed Jun 11, 2025
View reviewed changes
Comment thread Modules/grpmodule.c
struct group *p;
// `d` is a local list; append items without a lock using
// _PyList_AppendTakeRef() within the loop.
assert(_PyObject_IsUniquelyReferenced(d));

@kumaraditya303 kumaraditya303 Jun 11, 2025

Copy link
Copy Markdown
Contributor

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@kumaraditya303

kumaraditya303 commented Jun 11, 2025

Copy link
Copy Markdown
Contributor

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@yoney

yoney commented Jun 11, 2025

Copy link
Copy Markdown
Contributor Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

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

yoney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Yhg1s

Yhg1s commented Jun 12, 2025

Copy link
Copy Markdown
Member

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

yoney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Yhg1s Yhg1s closed this Jun 12, 2025
@yoney
yoney deleted the ft_grpmodule branch June 12, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@Yhg1s Yhg1s Yhg1s approved these changes
@colesbury colesbury Awaiting requested review from colesbury
@mpage mpage Awaiting requested review from mpage

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@yoney @mpage @kumaraditya303 @Yhg1s @ZeroIntensity
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.