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

Revamp section describing how to fix 1:nthreads() pattern #137

Merged
merged 3 commits into from
Feb 24, 2025

Conversation

MasonProtter
Copy link
Member

@MasonProtter MasonProtter commented Feb 21, 2025

Closes #136

This PR fixes the section describing 1:nthreads() anti-pattern, and adds a note that it will error in v1.12 by default, not just if someone purposefully starts julia with an interactive thread. The fix described now uses maxthreadid(), and has more info on why we don't think it's a good idea.

@carstenbauer if you approve of these changes, can you run the docs build pipeline on your machine again so it doesn't change a bunch from being run on my machine?

@MasonProtter MasonProtter changed the title Remove section describing the partial fix to 1:nthreads() pattern Revamp section describing the partial fix to 1:nthreads() pattern Feb 21, 2025
@MasonProtter MasonProtter changed the title Revamp section describing the partial fix to 1:nthreads() pattern Revamp section describing how to fix 1:nthreads() pattern Feb 21, 2025
@MasonProtter
Copy link
Member Author

MasonProtter commented Feb 21, 2025

cc @KristofferC. If we go with these changes, you can add a link to this doc section in your bullet point about maxthreadid() in #136 (comment)

@KristofferC
Copy link

👍

@KristofferC
Copy link

What do you think about adding a section with OncePerFunction as well?

@MasonProtter
Copy link
Member Author

Yeah definitely. Once v1.12 is out, that should probably be the preferred solution we advertise.

Before 1.12 is out, I'd be fine including sections on them, but I'm not sure the best place to put it, since it's not really actionable or helpful for the vast majority of users yet.

@KristofferC
Copy link

FYI, I have opened issues on the relevant repos now.

@MasonProtter MasonProtter merged commit b107907 into master Feb 24, 2025
14 of 15 checks passed
@MasonProtter
Copy link
Member Author

Okay, here's a link to those fixed docs if you need it: https://juliafolds2.github.io/OhMyThreads.jl/dev/literate/tls/tls/#The-naive-(and-incorrect)-approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "quick fix with caveats" example is not correct
2 participants