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-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe by corona10 · Pull Request #114843 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Feb 1, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Make list objects thread-safe in --disable-gil builds #112087

Sorry, something went wrong.

Uh oh!

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

All reactions
@corona10
pythongh-112087: Make __sizeof__ and listiter_{len, next} to be threa…
7186522
…dsafe
@corona10
corona10 requested a review from colesbury February 1, 2024 04:07
@bedevere-app bedevere-app Bot mentioned this pull request Feb 1, 2024
Make list objects thread-safe in --disable-gil builds #112087
Closed
4 tasks done
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 1, 2024
@corona10 corona10 added skip issue topic-free-threading and removed awaiting core review labels Feb 1, 2024
@corona10

corona10 commented Feb 1, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@colesbury

  • listiter_reduce_general looks safe, so I think that we don't have to touch it.
  • The rest of the APIs can be migrated once the gh-114329: Add PyList_GetItemRef function #114504 is merged.
All reactions

Sorry, something went wrong.

Uh oh!

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

@corona10 corona10 added skip news and removed skip issue labels Feb 1, 2024
corona10 added 2 commits February 1, 2024 13:12
@corona10
Fix typo
e8be089
@corona10
nit
777c9d5
@corona10
corona10 requested a review from DinoV February 1, 2024 05:30
@corona10 corona10 mentioned this pull request Feb 2, 2024
gh-112087: Make list_{count, index, contains} to be thread-safe. #114916
Merged
colesbury
colesbury reviewed Feb 2, 2024
View reviewed changes

@colesbury colesbury left a comment

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.

Regarding iterators: list (and dict) iterators current clear it->it_seq field when the iterator is exhausted. This will make it harder to make the iterators thread-safe because it_seq is mutable. In the free-threaded build, I think we should avoid clearing the it_seq field until the iterator is deallocated.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/listobject.c Outdated
Comment on lines +23 to +25
#ifdef Py_GIL_DISABLED
#define LOAD_SSIZE_ATOMIC_AS_POSSIBLE(value) _Py_atomic_load_ssize_relaxed(&value)
#define STORE_SSIZE_ATOMIC_AS_POSSIBLE(value, new_value) _Py_atomic_store_ssize_relaxed(&value, new_value)

@colesbury colesbury Feb 2, 2024

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.

The name is a bit confusing to me -- I'm not sure what AS_POSSIBLE means here. Maybe just LOAD_SSIZE(). It's not very descriptive, but at least it's short.

The style I've seen more commonly in CPython looks like:

#ifdef Py_GIL_DISABLED
#  define LOAD_SSIZE_ATOMIC_AS_POSSIBLE(value) _Py_atomic_load_ssize_relaxed(&value)
#endif

Sorry, something went wrong.

Uh oh!

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

corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@corona10
Address code review
671a1bc
@corona10
corona10 requested a review from colesbury February 3, 2024 10:38
@corona10 corona10 changed the title gh-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe [WIP] gh-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe Feb 5, 2024
@mpage mpage mentioned this pull request Feb 6, 2024
gh-115041: Add wrappers that are atomic only in free-threaded builds #115046
Merged
@corona10

corona10 commented Feb 6, 2024

Copy link
Copy Markdown
Member Author

Regarding iterators: list (and dict) iterators current clear it->it_seq field when the iterator is exhausted. This will make it harder to make the iterators thread-safe because it_seq is mutable. In the free-threaded build, I think we should avoid clearing the it_seq field until the iterator is deallocated.

@colesbury
I will update the implementation through this PR after #114916 is merged :)

All reactions

Sorry, something went wrong.

Uh oh!

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

corona10 added 2 commits February 7, 2024 15:50
@corona10
Merge remote-tracking branch 'upstream/main' into pythongh-112087-part3
61362aa
@corona10
Address code review
a93faf6
@corona10
corona10 requested review from gvanrossum and markshannon as code owners February 8, 2024 06:02
@corona10
corona10 marked this pull request as draft February 8, 2024 06:04
corona10
corona10 commented Feb 8, 2024
View reviewed changes
Comment thread Lib/test/test_iter.py
self.assertEqual(
run("reversed", orig["reversed"](list(range(8)))),
(iter, ([],))
(reversed, ([],))

@corona10 corona10 Feb 8, 2024

Copy link
Copy Markdown
Member Author

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.

This test was added from #101769, not sure that it will be okay to change the implementation detail.

Sorry, something went wrong.

Uh oh!

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

All reactions

@carljm carljm Feb 8, 2024

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 think it is OK, see my other comment.

Sorry, something went wrong.

Uh oh!

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

JelleZijlstra and corona10 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@corona10 corona10 changed the title [WIP] gh-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe gh-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe Feb 8, 2024
@corona10
corona10 marked this pull request as ready for review February 8, 2024 07:30
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 8, 2024
@corona10

corona10 commented Feb 8, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@colesbury
#114843 (review)
I addressed your code review, including the change of implementation details when the iterator is exhausted.

Since it touched on the implementation details, I think that we should listen to other core developers' opinions, too.

  1. Change it from CPython 3.13 for the default build and free-threaded build
  2. Only change it for a free-threaded build (Be more complex but possible option)

cc @carljm @JelleZijlstra

@gvanrossum
I only touched the tier 1 opcode, but if we need to touch the tier 2 opcode too, please let me know.
The refleak test for tier 2 of test_list and test_iter had already failed in the main branch.

Tier 1

$> ./python.exe -m test test_list test_iter -R 3:3
Using random seed: 1891590703
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 5.97 Run 2 tests sequentially
0:00:00 load avg: 5.97 [1/2] test_list
beginning 6 repetitions
123456
......
0:00:01 load avg: 5.73 [2/2] test_iter
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

All 2 tests OK.

Total duration: 1.9 sec
Total tests: run=114
Total test files: run=2/2
Result: SUCCESS

Tier 2

$ ./python.exe -Xuops -m test test_list test_iter -R 3:3
Using random seed: 542131965
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 6.22 Run 2 tests sequentially
0:00:00 load avg: 6.22 [1/2] test_list
beginning 6 repetitions
123456
......
test_list leaked [4, 3, 2] references, sum=9
test_list leaked [6, 4, 4] memory blocks, sum=14
0:00:01 load avg: 6.22 [2/2/1] test_iter -- test_list failed (reference leak)
beginning 6 repetitions
123456
......
test_iter leaked [8, 9, 6] references, sum=23
test_iter leaked [12, 11, 8] memory blocks, sum=31
test_iter failed (reference leak)

== Tests result: FAILURE ==

2 tests failed:
    test_iter test_list

Total duration: 2.0 sec
Total tests: run=114
Total test files
All reactions

Sorry, something went wrong.

Uh oh!

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

carljm
carljm reviewed Feb 8, 2024
View reviewed changes
Comment thread Objects/listobject.c
if (list == NULL)
return NULL;
return Py_BuildValue("N(N)", _PyEval_GetBuiltin(&_Py_ID(iter)), list);
return Py_BuildValue("N(N)", iter, list);

@carljm carljm Feb 8, 2024

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.

This is the change causing the behavior difference in the test. Previously, an empty iterator would always reduce to the iter builtin, now it will reduce to reversed if it was a reverse iterator. IMO the new behavior is more correct (consistent with the non-empty behavior) and we can consider this a bugfix.

Sorry, something went wrong.

Uh oh!

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

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

@corona10 corona10 Feb 9, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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.

Okay in that case, we need to create backport patches for this.

Sorry, something went wrong.

Uh oh!

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

carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
colesbury
colesbury reviewed Feb 8, 2024
View reviewed changes
Comment thread Objects/listobject.c Resolved

Uh oh!

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

@colesbury colesbury mentioned this pull request Feb 8, 2024
gh-112075: Iterating a dict shouldn't require locks #115108
Merged
corona10 added 3 commits February 9, 2024 09:20
@corona10
Address code review
44f2022
@corona10
nit
79989db
@corona10
fix
f4526f9
@corona10

corona10 commented Feb 13, 2024

Copy link
Copy Markdown
Member Author

@gvanrossum @colesbury gentle ping :)

All reactions

Sorry, something went wrong.

Uh oh!

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

colesbury
colesbury reviewed Feb 13, 2024
View reviewed changes

@colesbury colesbury left a comment

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.

I think _GUARD_NOT_EXHAUSTED_LIST also needs to be adjusted for the free-threaded build because seq == NULL no longer signifies an exhausted iterator for the free-threaded build. Like with _ITER_JUMP_LIST, I think the deopt can be reduced to:

DEOPT_IF((size_t)it->it_index >= (size_t)PyList_GET_SIZE(seq));

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/listobject.c Resolved

Uh oh!

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

Comment thread Objects/listobject.c Outdated
Comment on lines +23 to +24
#ifdef Py_GIL_DISABLED
#define LOAD_SSIZE(value) _Py_atomic_load_ssize_relaxed(&value)

@colesbury colesbury Feb 13, 2024

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.

The indentation I've seen more commonly in CPython looks like:

#ifdef Py_GIL_DISABLED
#  define LOAD_SSIZE(value) _Py_atomic_load_ssize_relaxed(&value)
...
#endif

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/listobject.c Resolved

Uh oh!

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

Comment thread Python/bytecodes.c Resolved

Uh oh!

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

@gvanrossum

gvanrossum commented Feb 13, 2024

Copy link
Copy Markdown
Member

@gvanrossum @colesbury gentle ping :)

I trust Sam. My queue is very long and my proessing capacity is reduced.

corona10 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.

@corona10
Address code review
a13beed
@corona10 corona10 removed the skip news label Feb 14, 2024
@corona10
Address code review
bec617a
@corona10
corona10 force-pushed the gh-112087-part3 branch 2 times, most recently from e91da7e to bec617a Compare February 14, 2024 15:30
@corona10
Address code review
6e170cc
@corona10
corona10 requested a review from colesbury February 14, 2024 15:48
colesbury
colesbury reviewed Feb 14, 2024
View reviewed changes
Comment thread Python/bytecodes.c Resolved

Uh oh!

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

Comment thread Python/bytecodes.c Resolved

Uh oh!

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

@corona10
Address code review
c8a8b81
@corona10
corona10 requested a review from colesbury February 14, 2024 15:58
colesbury
colesbury approved these changes Feb 14, 2024
View reviewed changes

@colesbury colesbury left a comment

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.

LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Feb 14, 2024
@corona10
corona10 merged commit a2d4281 into python:main Feb 14, 2024
@corona10
corona10 deleted the gh-112087-part3 branch February 14, 2024 17:00
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 14, 2024
@colesbury colesbury mentioned this pull request Feb 20, 2024
gh-115733: Fix crash involving exhausted list iterator #115740
Merged
@eendebakpt eendebakpt mentioned this pull request Jan 29, 2025
gh-123471: make itertools.batched thread-safe #129416
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm left review comments
@colesbury colesbury colesbury approved these changes
@DinoV DinoV Awaiting requested review from DinoV
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@gvanrossum gvanrossum Awaiting requested review from gvanrossum

Uh oh!

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

Assignees

No one assigned

Labels

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.

4 participants

@corona10 @gvanrossum @carljm @colesbury
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.