8000 Draft: gh-123471: Make concurrent iteration over itertools.pairwise safe under free-threading by eendebakpt · Pull Request #125417 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

Draft: gh-123471: Make concurrent iteration over itertools.pairwise safe under free-threading #125417

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Draft: gh-123471: Make concurrent iteration over itertools.pairwise safe under free-threading #125417

wants to merge 5 commits into from
+72 −9

Conversation

eendebakpt
Copy link
Contributor
@eendebakpt eendebakpt commented Oct 13, 2024 •
edited
Loading

We make concurrent iteration over itertools.pairwise safe by:

  • Using _PyObject_IsUniquelyReferenced for re-use of the result type
  • Holding a reference to po->it instead of a borrowed reference
  • Obtaining a reference to po->old early.

Also see #123848

To make pairwise_next safe we need to prevent concurrent iterators from accessing the cleared po->it

  • Idea 1: In the first line of pairwise_next replace PyObject *it = po->it; with PyObject *it = Py_XInRef(po->it);.
    Then every thread will have a true reference to the iterator, and not a borrowed refe 8000 rence.
    Simple, but overhead is an incref/decref in the common case
  • Idea 2a: Do not clear po->it. We then need another way to signal that the iterator has been exhausted.
    Use another variable "int iterator_exhausted" in the pairwise object. Requires a bit more memory
    for each pairwise object, but the check is a simple int comparison.
  • Idea 2b: Do not clear po->it. We then need another way to signal that the iterator has been exhausted.
    Use po->old to set a sentinel. Requires a global sentinel in the module (or a sentinel per thread).
    In the top of pairwise_next we would have
PyObject *old = Py_XNewRef(po->old);
if (old == sentinel) {
      // iterator was exhausted earlier, return NULL
      Py_DECREF(old)
      return NULL;
}
if (old == NULL) {
      // first call to pairwise_next, normal code
      ...
}

An issue with this approach is that at the end of pairwise_next there is the line: Py_XSETREF(po->old, new);
So one thread can set the sentinel, but in another thread the value could be overwritten.
Maybe this is ok: this will not cause crashes, and we do not have to guarantee correct behaviour for
the free-threading build.
This approach does not require additional increfs/decrefs, but it does require setting up the sentinel.

In this PR we work out the first option.

@eendebakpt eendebakpt marked this pull request as draft October 13, 2024 21:22
@bedevere-app bedevere-app bot mentioned this pull request Oct 13, 2024
Open
@rhettinger
Copy link
Contributor

Can I suggest that you start with enumerate() rather than pairwise(). The former is a built-in, so more people care about it and your edit will get more attention, feedback, and buy-in.

Also, I would like to keep the latter as simple as possible for now. I'm evaluating a proposal to add prefix and suffix options to pairwise(). This would be much harder to do after these edits.

@eendebakpt
Copy link
Contributor Author

Can I suggest that you start with enumerate() rather than pairwise(). The former is a built-in, so more people care about it and

Sure! I created a PR for enumerate at #125734. For reversed there was an earlier PR #120971 where your review was requested. Would you have time to review that one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eendebakpt @rhettinger
0