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

Mark the _state field as atomic and move to proper atomics instead of llvmcall #55502

Merged
merged 5 commits into from
Aug 20, 2024

Conversation

gbaraldi
Copy link
Member

No description provided.

@vtjnash vtjnash added domain:multithreading Base.Threads and related functionality status:merge me PR is reviewed. Merge when all tests are passing labels Aug 15, 2024
@giordano
Copy link
Contributor

https://buildkite.com/julialang/julia-master/builds/39245#019157be-da72-4fb0-9294-7835a6bdcbff/830-1686

Error in testset Serialization:
Error During Test at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-HL2F7YQ3XH.0/build/default-honeycrisp-HL2F7YQ3XH-0/julialang/julia-master/julia-3eb9ab3e1c/share/julia/test/testdefs.jl:24
  Got exception outside of a @test
  LoadError: ConcurrencyViolationError("setfield!: atomic field cannot be written non-atomically")
  Stacktrace:
    [1] setproperty!
      @ ./Base.jl:53 [inlined]
    [2] setproperty!
      @ ./task.jl:193 [inlined]
    [3] deserialize(s::Serializer{IOBuffer}, ::Type{Task})
      @ Serialization /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-HL2F7YQ3XH.0/build/default-honeycrisp-HL2F7YQ3XH-0/julialang/julia-master/julia-3eb9ab3e1c/share/julia/stdlib/v1.12/Serialization/src/Serialization.jl:1575

looks relevant

@vchuravy
Copy link
Sponsor Member

Fixes #55125 (comment)

See #55125 in general for which tests to update. I think you need to simply adjust the serialization code to also use an atomic here.

@gbaraldi
Copy link
Member Author

Looking at the uses of _state most are unordered atomics so I won't change them. But they might deserve a look

@giordano
Copy link
Contributor

I presume this is good to go?

@gbaraldi gbaraldi merged commit f2f76d8 into master Aug 20, 2024
7 checks passed
@gbaraldi gbaraldi deleted the gb/stateatomic branch August 20, 2024 17:39
@giordano giordano removed the status:merge me PR is reviewed. Merge when all tests are passing label Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants