Skip to content
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

gh-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe #114843

Merged
merged 13 commits into from
Feb 14, 2024

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Feb 1, 2024

@corona10
Copy link
Member Author

corona10 commented Feb 1, 2024

@colesbury

Copy link
Contributor

@colesbury colesbury left a 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.

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)
Copy link
Contributor

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

@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
@corona10
Copy link
Member Author

corona10 commented Feb 6, 2024

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 :)

@corona10 corona10 marked this pull request as draft February 8, 2024 06:04
@@ -302,7 +302,7 @@ def __eq__(self, other):
# listiter_reduce_general
self.assertEqual(
run("reversed", orig["reversed"](list(range(8)))),
(iter, ([],))
(reversed, ([],))
Copy link
Member Author

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.

Copy link
Member

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.

@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
@corona10
Copy link
Member Author

corona10 commented Feb 8, 2024

@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

}
/* empty iterator, create an empty list */
list = PyList_New(0);
if (list == NULL)
return NULL;
return Py_BuildValue("N(N)", _PyEval_GetBuiltin(&_Py_ID(iter)), list);
return Py_BuildValue("N(N)", iter, list);
Copy link
Member

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.

Copy link
Member Author

@corona10 corona10 Feb 9, 2024

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.

@corona10
Copy link
Member Author

@gvanrossum @colesbury gentle ping :)

Copy link
Contributor

@colesbury colesbury left a 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));

Objects/listobject.c Outdated Show resolved Hide resolved
Comment on lines 23 to 24
#ifdef Py_GIL_DISABLED
#define LOAD_SSIZE(value) _Py_atomic_load_ssize_relaxed(&value)
Copy link
Contributor

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

Objects/listobject.c Outdated Show resolved Hide resolved
Python/bytecodes.c Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member

@gvanrossum @colesbury gentle ping :)

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

@corona10 corona10 force-pushed the gh-112087-part3 branch 2 times, most recently from e91da7e to bec617a Compare February 14, 2024 15:30
@corona10 corona10 requested a review from colesbury February 14, 2024 15:48
Python/bytecodes.c Outdated Show resolved Hide resolved
Python/bytecodes.c Outdated Show resolved Hide resolved
@corona10 corona10 requested a review from colesbury February 14, 2024 15:58
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@corona10 corona10 merged commit a2d4281 into python:main Feb 14, 2024
53 checks passed
@corona10 corona10 deleted the gh-112087-part3 branch February 14, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants