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

Change default to -t auto on master #50636

Open
PallHaraldsson opened this issue Jul 22, 2023 · 8 comments
Open

Change default to -t auto on master #50636

PallHaraldsson opened this issue Jul 22, 2023 · 8 comments
Labels
multithreading Base.Threads and related functionality speculative Whether the change will be implemented is speculative

Comments

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Jul 22, 2023

I'm proposing -t auto so that people don't have to opt-into (parallel) performance, rather could opt out of the (very rare in practice) non-determinism with -t 1.

The PR for master should be trivial, and I realize we are past feature freeze for 1.10, but this doesn't seem like a new feature, rather an existing non-default feature being promoted to default. My thinking is 1.10 will become LTS, and it would be best to have this in from the start. I'm willing to write the NEWS (unless the proposal here will be totally shot down).

And even if we see problems on 1.10 (or master), e.g. with PkgEvel, I think it best for users to see sooner rather than later, so would be great for 1.10-alpha2, at least in (or before) rc1.

I asked what people think on discourse, and actually -t auto is the default in VS Code, so got support, and I explained why I think fears are misplaced:
https://discourse.julialang.org/t/why-is-t-auto-not-the-default-for-auto-threading-and-what-to-do-about-it-would-users-want-it-by-default/101858/14?u=palli

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Jul 24, 2023

@t-bltg what's your objection if not what I post below? I don't know you and since Oscar gave a thumbs up I continued to investigate a PR. I also respect the other users with confused look, just unsure what is confusing (I want simpler sane defaults, e.g. when a Windows user clicks on a Julia file to run it).

Is this warning still valid (possibly too conservative or outdated, since VS Code starts with not 1 default thread)?

When you start Julia in multithreaded mode with `JULIA_NUM_THREADS=X`, it is generally recommended to set `OPENBLAS_NUM_THREADS=1`.

@fonsp, Pluto sets (i.e. caps at) 16 threads for some reason:
fonsp/Pluto.jl#1175

@vchuravy
Copy link
Member

This is a feature change. We will not backport it to 1.10.

There are large pieces in Julia that are not yet fully thread-safe,
so while I do think we should do this eventually, this is premature.

@vchuravy vchuravy added the speculative Whether the change will be implemented is speculative label Jul 24, 2023
@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Jul 24, 2023

Are you open to this on master at least?

There are large pieces in Julia that are not yet fully thread-safe

You presumably mean in the Julia ecosystem (despite this threading default in Pluto and VS Code); or in Julia itself? I'm not in a hurry if will not be accepted for 1.10. I think it should go in there if a trivial change though, so at least merge on master to see if people object to being a safe change?

@KristofferC
Copy link
Member

I think it should go in there if a trivial change though, so at least merge on master to see if people object to being a safe change?

The reason this has not been done is not really that it is hard to swap the default but that julia is not considered thread safe enough yet. For example, when threading was enabled on CI there were many issues that popped up.

@vchuravy
Copy link
Member

Before we can consider this issue we need to first address #49778 as well as JuliaLang/Distributed.jl#73.

You presumably mean in the Julia ecosystem

No in the stdlibs and Julia Base.

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Jul 25, 2023

julia is not considered thread safe enough yet.

I just realized multi-threaded Julia will never be (fully) thread-safe; e.g. with -t auto done manually with current Julia nor with my PR nor even is the -t 1 current default -t 1 totally safe, without making Julia more Rust-like (ccall is also a problem). But I still want the new default, and can justify it, with some changes, though not ASAP for 1.10.

@JeffBezanson claimed Distributed is now thread-safe (and @jonas-schulze showed in June it wasn't actually achieved JuliaLang/Distributed.jl#73), and that package loading is now thread-safe.

So, @NHDaly what are include and package_callbacks? Both described as "experimental", but I don't understand the implication, possibly broken currently if multi-threading enabled?

julia/base/loading.jl

Lines 1551 to 1557 in 5039d8a

# to notify downstream consumers that a module was successfully loaded
# Callbacks take the form (mod::Base.PkgId) -> nothing.
# WARNING: This is an experimental feature and might change later, without deprecation.
const package_callbacks = Any[]
# to notify downstream consumers that a file has been included into a particular module
# Callbacks take the form (mod::Module, filename::String) -> nothing
# WARNING: This is an experimental feature and might change later, without deprecation.

Should we be worried, since a lot of users run Julia with -t auto (as in VS Code), and in Pluto with 16 threads by default?

It seems like if this was having a real effect (on many users) we would know by now. If this is a real worry, we should warn against the thread option, do we currently?

I think multi-threading should be in the hands of the programmer, not for the user to enable it, though I like having the option for the user to disable it, also sort of as a debug mode.

Since the programmer can't add threads (at least currently, from Julia, I still believe, though there's a possibility by now indirectly, by calling C code that adds threads that Julia adopts), I think Julia should start with a sensible max amount of threads, and the programmer can choose to not use more than 1 of them (which is sort of the case already, since threads are opt-in locally).

Would it help to add a new stdlib module, that has access to all those threads safely, and the default .Threads module would still be limited to 1:

using SafeThreads

counter = 0
Threads.@threads for _ in 1:10_000
  counter +=1
end

I.e. that code example from here (the video at the top), would now fail at compile time ideally, if not possible, at runtime with an Exception:

https://discourse.julialang.org/t/blog-post-rust-vs-julia-in-scientific-computing/101711/11?u=palli

I'm thinking SafeThreads would allow the same Threads.@threads (for all the correct code out there, also likely allowing just @threads since should be exported from SafeThreads), just but then it couldn't refer to global variables nor write to (ok to read?) local variables, e.g. counter. And it should disallow ccall in the loop, or possibly calling any function, at least for now, since it might do a ccall. Composable threading seemed like a great idea at the time, but I understand it's broken, and the API no know bad with threadid() and nthreads(), and the new SafeThreads could skip those and have a correct subset API? For now non-composable until figured out. At some point the older Threads could be deprecated for 2.0, then removed, and maybe end up a synonym for SafeThreads in 2.0?

https://julialang.org/blog/2023/07/PSA-dont-use-threadid/

No in the stdlibs and Julia Base.

We could fix all those problems and users would still not be safe, because of threading in user code; or even be safe with -t 1 because such code can ccall and add threads.

when threading was enabled on CI there were many issues that popped up.

@PallHaraldsson PallHaraldsson changed the title Change default to -t auto on master; and backport to 1.10 ASAP Change default to -t auto on master Jul 25, 2023
@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Jul 25, 2023

Since Distributed is a parallel mechanism, an alternative to Threads, and known to be broken when used to together currency (despite a supposed fix, also questionably useful together even if not broken per se), I suggest when using -p it will imply -t 1, at least for now, unless -t explicitly invoked.

Would this make accepting my PR (for master only for now), with such a change, more plausible?

@brenhinkeller brenhinkeller added the multithreading Base.Threads and related functionality label Aug 6, 2023
@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Aug 31, 2023

@KristofferC, since you closed the PR (and I agree with that, it wasn't that simple, if we want to be safe), should this issue stay open?

I now know the issues with the proposal, and the/a way around it. I believe @oscardssmith upvoted here for the goal. But the goal can't be to only enable threads by default. It needs to be enable (as the PR did, sort of), but not use them by default, only to have them available if the programmer additionally opts into using. We will never be fully thread-safe, if you consider not just the Julia standard library, but also e.g. ccall. Since in my analysis, we can never do what the title proposes, we need to at least change the title here, or close here and open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality speculative Whether the change will be implemented is speculative
Projects
None yet
Development

No branches or pull requests

4 participants