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

Don't use threadid in e.g. BBCode #391

Open
gdalle opened this issue Nov 25, 2024 · 3 comments
Open

Don't use threadid in e.g. BBCode #391

gdalle opened this issue Nov 25, 2024 · 3 comments
Labels
bug Something isn't working low priority

Comments

@gdalle
Copy link
Collaborator

gdalle commented Nov 25, 2024

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

@willtebbutt
Copy link
Member

So the specific bit of a code with a problem here is this:

const _id_count::Dict{Int,Int32} = Dict{Int,Int32}()

"""
    ID()

An `ID` (read: unique name) is just a wrapper around an `Int32`. Uniqueness is ensured via a
global counter, which is incremented each time that an `ID` is created.

This counter can be reset using `seed_id!` if you need to ensure deterministic `ID`s are
produced, in the same way that seed for random number generators can be set.
"""
struct ID
    id::Int32
    function ID()
        current_thread_id = Threads.threadid()
        id_count = get(_id_count, current_thread_id, Int32(0))
        _id_count[current_thread_id] = id_count + Int32(1)
        return new(id_count)
    end
end

which can be found here.

I think the only problem that was causing problems here previously was race conditions, in which multiple threads would attempt to access and increment the _id_count const (which was previously just an Int, rather than a Dict mapping threads to Ints). I believe that the above solves this, because there's no risk of race conditions. I could also have made use of atomic operations or a lock of some kind, and it would have been fine.

This doesn't solve the determinism problem though - it's quite helpful when debugging to have it such that each time you build the same rule, you get the same set of IDs being used for the same things each time. I suspect that I've not encountered this in practice because I've not stress tested this a huge amount on multi-threaded code.

Would indexing by current_task() work? i.e. my understanding is that a given Task can run on any available thread, and have its work moved between threads at arbitrary points in time during execution, but that the Task itself is independent of the thread that it's run on.

@willtebbutt willtebbutt added invalid This doesn't seem right bug Something isn't working low priority and removed invalid This doesn't seem right labels Nov 25, 2024
@gdalle
Copy link
Collaborator Author

gdalle commented Nov 25, 2024

TBH I'm not completely comfortable with the task/thread distinction, I just signaled it because I had the blog post title in mind

@willtebbutt
Copy link
Member

Cool. I'll ask around a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low priority
Projects
None yet
Development

No branches or pull requests

2 participants