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-120496: Use CAS approach for rangeiter_next by corona10 · Pull Request #120534 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Jun 15, 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: Sequence iterator thread-safety #120496

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot mentioned this pull request Jun 15, 2024
Sequence iterator thread-safety #120496
Closed
@corona10
corona10 requested review from Fidget-Spinner and colesbury June 15, 2024 01:08
@corona10 corona10 added the DO-NOT-MERGE label Jun 15, 2024
corona10 added 2 commits June 15, 2024 10:18
@corona10
pythongh-120496: Use CAS approach for rangeiter_next
eb7674b
@corona10
nit
2d9ac70
@corona10 corona10 added the skip news label Jun 15, 2024
@corona10
nit
a547e9d
@corona10
corona10 force-pushed the gh-120496 branch from ef235dd to a547e9d Compare June 15, 2024 01:22
corona10 added 5 commits June 15, 2024 10:23
@corona10
fix
6ee317a
@corona10
More optimization
fd95922
@corona10
Update atomic package
901ea19
@corona10
fix
827d38b
@corona10
fix
805a90e
@corona10 corona10 removed the DO-NOT-MERGE label Jun 15, 2024
@corona10
corona10 marked this pull request as ready for review June 15, 2024 03:12
@bedevere-app bedevere-app Bot added the awaiting core review label Jun 15, 2024
@corona10 corona10 added needs backport to 3.13 only security fixes and removed awaiting core review labels Jun 15, 2024
@Fidget-Spinner

Fidget-Spinner commented Jun 15, 2024

Copy link
Copy Markdown
Member

Thanks for doing this! Do you have any benchmark results on how much the perf impact is? versus say using Py_BEGIN_CRITICAL_SECTION ?

All reactions

Sorry, something went wrong.

Uh oh!

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

@corona10

corona10 commented Jun 15, 2024

Copy link
Copy Markdown
Member Author

Thanks for doing this! Do you have any benchmark results on how much the perf impact is? versus say using

I will try to prepare :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@corona10

corona10 commented Jun 15, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@Fidget-Spinner @colesbury

I am not sure this will be a fair benchmark, but it shows different strengths in terms of technique and size of N.

Single thread

import pyperf

runner = pyperf.Runner()
runner.timeit(name="bench iter single",
              stmt="""
data = list(it)
""",
              setup = """
N = 100
it = iter(range(N))
"""
              )
  • pyperf compare_to single_base.json single_cas.json
    Benchmark hidden because not significant (1): bench iter single

Mutl thread

import pyperf

runner = pyperf.Runner()
runner.timeit(name="bench iter multi",
              stmt="""
for _ in range(100):
    it = iter(range(N))
    with concurrent.futures.ThreadPoolExecutor() as executor:
        data = list(executor.map(lambda _: next(it), range(N)))
""",
              setup = """
import concurrent.futures
N = 100
"""
              )
  • Mean +- std dev: [multi_baseline] 284 ms +- 11 ms -> [multi_cas] 277 ms +- 20 ms: 1.03x faster
All reactions

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner

Fidget-Spinner commented Jun 15, 2024

Copy link
Copy Markdown
Member

Hmmm the perf difference is not that large. That might imply that we should just use critical section for simplicity, and hope that someone on PyPI publishes a "prange()" or something like that.

All reactions

Sorry, something went wrong.

Uh oh!

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

@corona10

corona10 commented Jun 15, 2024

Copy link
Copy Markdown
Member Author

That might imply that we should just use critical section for simplicity, and hope that someone on PyPI publishes a "prange()" or something like that.

Yeah, I prefer this one.

All reactions

Sorry, something went wrong.

Uh oh!

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

eendebakpt
eendebakpt reviewed Jun 15, 2024
View reviewed changes
Comment thread Objects/rangeobject.c
if (len <= 0) {
return NULL;
}
long result = _Py_atomic_load_long_relaxed(&r->start);

@eendebakpt eendebakpt Jun 15, 2024 •
edited
Loading

Uh oh!

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

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.

What if we have a range object with len=1 and many threads simultaneously at this point? The check on len has already been done, so the threads each increment the value of start and we end up with a value for result larger than end.

Sorry, something went wrong.

Uh oh!

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

All reactions

@corona10 corona10 Jun 15, 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.

Maybe fail at _Py_atomic_compare_exchange_long?

Sorry, something went wrong.

Uh oh!

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

All reactions

@corona10 corona10 Jun 15, 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.

Ah.. yes understood what you pointed out.

Sorry, something went wrong.

Uh oh!

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

All reactions

@corona10 corona10 Jun 15, 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.

There will be a slight timing issue. Then yes, let's just use the critical section that @Fidget-Spinner also preferred.

Sorry, something went wrong.

Uh oh!

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

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

@corona10 corona10 Jun 15, 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.

To prepare for the overcommitted issue, we can validate the value of the result by comparing the end value.
But let's just use critical section approach.

Sorry, something went wrong.

Uh oh!

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

All reactions
@corona10

corona10 commented Jun 15, 2024

Copy link
Copy Markdown
Member Author

See: #120540

All reactions

Sorry, something went wrong.

Uh oh!

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

@corona10 corona10 closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eendebakpt eendebakpt eendebakpt left review comments
@colesbury colesbury Awaiting requested review from colesbury
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner

Uh oh!

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

Assignees

No one assigned

Labels

needs backport to 3.13 only security fixes skip news

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.

3 participants

@corona10 @Fidget-Spinner @eendebakpt
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 01402520 — 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.