Skip to content

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Mar 10, 2022

This appears to be the only remaining place we use the nthreads variable
to pre-initialize a shared array. Let us fix that code pattern now,
since we know globals are already acquire/release.

@vtjnash vtjnash added the multithreading Base.Threads and related functionality label Mar 10, 2022
@vtjnash vtjnash requested review from tkf and JeffBezanson March 10, 2022 00:39
Copy link
Member

@tkf tkf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation itself LGTM.

This appears to be the only remaining place we use the nthreads variable to pre-initialize a shared array. Let us fix that code pattern now,

Why does this code pattern have to be fixed? Using nthreads for "pseudo thread-local storage" is a bad idea for various parallelism patterns like parallel for loops. But I do think there are legit use cases, especially in concurrent data structures where knowing the number of worker threads is beneficial. For example, it's very useful for writing object/memory pools that can be used in generic concurrent programs. On the theory side, I think shared register construction is a good example that shows knowing something like nthreads is useful.

Instead of hard-coding a thread-local storage pattern like this, I think it'd be better to have a thread-local storage API in Base.Threads. Maybe something like

Threads.@local_storage(THREAD_MATCH_CONTEXTS) do
    create_match_context()
end

get_local_match_context() = THREAD_MATCH_CONTEXTS[]

where Threads.@local_storage is a macro instead of a function call so that we can use import-time allocation via __init__ in a private module 1 as an implementation strategy.

Footnotes

  1. Here's an implementation of this strategy I'm using for Recyclers.@global.

_nth() = Int(unsafe_load(cglobal(:jl_n_threads, Cint)))

function get_local_match_context()
tid = _tid()
ctx = @inbounds THREAD_MATCH_CONTEXTS[tid]
ctxs = THREAD_MATCH_CONTEXTS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it scary that we rely on "implicit" atomic ordering of global for code like this. It looks so innocent and the nuance can get lost during refactoring (even though I understand it is unlikely to happen for this part of code). Rather, I think it'd be easier to encode the atomic ordering by doing something like const THREAD_MATCH_CONTEXTS = Threads.Atomic(Ptr{Cvoid}[C_NULL]).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am just waiting on #44231 to be able to specify it explicitly. For now, I am okay with relying on the specified behavior implicitly.

This appears to be the only remaining place we use the nthreads variable
to pre-initialize a shared array. Let us fix that code pattern now,
since we know globals are already acquire/release.
@vtjnash
Copy link
Member Author

vtjnash commented Mar 11, 2022

Instead of hard-coding a thread-local storage pattern like this, I think it'd be better to have a thread-local storage API in Base.Threads. Maybe something like

Threads.@local_storage(THREAD_MATCH_CONTEXTS) do
    create_match_context()
end

get_local_match_context() = THREAD_MATCH_CONTEXTS[]

This sounds like you are preparing to talk us into #35833 😂

@vtjnash vtjnash force-pushed the jn/axe-resize_nthreads branch from 6a65b64 to 85c8b6d Compare March 11, 2022 19:08
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Mar 11, 2022
@tkf
Copy link
Member

tkf commented Mar 12, 2022

I don't consider the discussion on nthreads resolved (ref #44542 (comment)), but I guess that's OK since this PR only changes the implementation to the one robust against future changes in many directions.

@vtjnash vtjnash merged commit ca17788 into master Mar 15, 2022
@vtjnash vtjnash deleted the jn/axe-resize_nthreads branch March 15, 2022 16:52
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Mar 15, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants