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-117657: Add TSAN suppressions for the free-threaded build by mpage · Pull Request #117736 · python/cpython

Conversation

@mpage

@mpage mpage commented Apr 10, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Add TSAN suppressions for existing races in the free-threaded build.

Additionally, skip a few tests that take a prohibitively long amount of time (> 1 hour) when TSAN is enabled and the GIL is disabled. I'm not sure why these are so much slower with TSAN (> 1 hour) vs without it (~1 minute). perf shows that all of the time is being spent in inclusively in GC in both configurations. When TSAN is enabled ~all of the exclusive cycles are in TSAN code (e.g __tsan_read8, __tsan_read1, etc.).

I cherry-picked this on top of the nogil-integration branch and verified that the free-threading TSAN tests pass locally and in CI.

  • Issue: Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@mpage
Add TSAN suppressions for the free-threaded build
509dac0 
Add TSAN suppressions for existing races in the free-threaded build.
@bedevere-app bedevere-app Bot mentioned this pull request Apr 10, 2024
Make TSAN tests pass with the GIL disabled in free-threaded builds #117657
Closed
@mpage mpage added the skip news label Apr 10, 2024
@mpage
Skip prohibitively slow tests
1d44c87
@mpage
mpage requested a review from colesbury April 11, 2024 00:13
@mpage
mpage marked this pull request as ready for review April 11, 2024 00:13
@mpage
mpage requested review from ezio-melotti and hugovk as code owners April 11, 2024 00:13
@bedevere-app bedevere-app Bot added the awaiting review label Apr 11, 2024
colesbury
colesbury reviewed Apr 11, 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.

Great!

Now that we have a list we can split up the work on fixing the data races.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/tsan/supressions.txt Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/tsan/suppressions_free_threading.txt Resolved

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
colesbury requested review from corona10 and pitrou April 11, 2024 00:33
@mpage

mpage commented Apr 11, 2024

Copy link
Copy Markdown
Contributor Author

Now that we have a list we can split up the work on fixing the data races.

Yep! I'll create issues and link them to #117657.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 approved these changes Apr 11, 2024
View reviewed changes

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

Well 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 review labels Apr 11, 2024
pitrou
pitrou reviewed Apr 11, 2024
View reviewed changes
Comment thread Lib/test/test_weakref.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
pitrou
pitrou reviewed Apr 11, 2024
View reviewed changes
Comment thread Tools/tsan/suppressions_free_threading.txt

## Free-threaded suppressions

race:_add_to_weak_set

@pitrou pitrou Apr 11, 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.

Are there GH issues open for all these suppressions?

Sorry, something went wrong.

Uh oh!

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

@mpage mpage Apr 11, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 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.

Not yet, working on it :) I want to provide a bit more context (i.e. sample stacktraces + repro instructions) rather than just cutting issues without context.

Sorry, something went wrong.

Uh oh!

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

@colesbury colesbury Apr 11, 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.

I'd like to start with a simple list in an omnibus issue like #117657 so that there's a single place that people can go look to track who is working on what races.

I expect many of the races to have simple fixes, but we can break off the more complex races into their own issues as needed.

The work of collecting stacktraces and repros is work that can be split up once this PR is landed and we have a way of coordinating the work.

Sorry, something went wrong.

Uh oh!

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

@mpage mpage Apr 11, 2024

Copy link
Copy Markdown
Contributor 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.

Sounds good. I threw together a quick script to massage the TSAN output from the tests into an index that we can use to look for stacktraces and tests that should reproduce the races.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
mpage added 2 commits April 11, 2024 11:23
@mpage
Categorize mimalloc races
8f18671
@mpage
Reduce iteration count instead of skipping tests
1ea8107
@colesbury colesbury mentioned this pull request Apr 11, 2024
Disable GIL by default in free-threaded build #116749
Closed
@mpage
mpage requested a review from pitrou April 11, 2024 21:51
colesbury
colesbury approved these changes Apr 15, 2024
View reviewed changes
@colesbury
colesbury merged commit 4783206 into python:main Apr 15, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 15, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@mpage @diegorusso
pythongh-117657: Add TSAN suppressions for the free-threaded build (p…
c267ff8 
…ython#117736)

Additionally, reduce the iterations for a few weakref tests that would
otherwise take a prohibitively long amount of time (> 1 hour) when TSAN
is enabled and the GIL is disabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes
@corona10 corona10 corona10 approved these changes
@ezio-melotti ezio-melotti Awaiting requested review from ezio-melotti ezio-melotti is a code owner
@hugovk hugovk Awaiting requested review from hugovk hugovk is a code owner
@pitrou pitrou Awaiting requested review from pitrou

Uh oh!

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

Assignees

No one assigned

Labels

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.

4 participants

@mpage @colesbury @pitrou @corona10
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.