JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
44.94k stars 5.42k forks source link

Change default to -t auto on master #50636

Open PallHaraldsson opened 11 months ago

PallHaraldsson commented 11 months ago

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 commented 11 months ago

@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)? https://github.com/JuliaLang/julia/blob/3cc05908a61aec885f3a4fc3fc9cb13cf3524108/doc/src/manual/performance-tips.md?plain=1#L1673

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

vchuravy commented 11 months ago

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.

PallHaraldsson commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

You presumably mean in the Julia ecosystem

No in the stdlibs and Julia Base.

PallHaraldsson commented 11 months ago

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 https://github.com/JuliaLang/Distributed.jl/issues/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?

https://github.com/JuliaLang/julia/blob/5039d8aceb4802cb40548b69fcb600bb5ac59985/base/loading.jl#L1551-L1557

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 commented 11 months ago

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?

PallHaraldsson commented 10 months ago

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