Skip to content

Implement PaddedSpinLock, which avoids false sharing.#55944

Merged
IanButterworth merged 13 commits intoJuliaLang:masterfrom
kuszmaul:master
May 11, 2025
Merged

Implement PaddedSpinLock, which avoids false sharing.#55944
IanButterworth merged 13 commits intoJuliaLang:masterfrom
kuszmaul:master

Conversation

@kuszmaul
Copy link
Copy Markdown
Contributor

@kuszmaul kuszmaul commented Sep 30, 2024 •
edited
Loading

Implemented PaddedSpinLock to help avoid false sharing.

Defines a new AbstractSpinLock with SpinLock <: AbstractSpinLock and
PaddedSpinLock <: AbstractSpinLock.

Fixes #55942.

@kuszmaul
Implement PaddedSpinLock, which avoids false sharing.
66d401c 
Defines a new `AbstractSpinLock` with `SpinLock <: AbstractSpinLock` and
`PaddedSpinLock <: AbstractSpinLock`.

Fixes 55942.
@nsajko nsajko added parallelism Parallel or distributed computation feature Indicates new feature / enhancement requests labels Sep 30, 2024
nsajko
nsajko reviewed Sep 30, 2024
View reviewed changes
Comment thread base/locks-mt.jl Outdated
@nsajko nsajko added needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Sep 30, 2024
@nsajko
Copy link
Copy Markdown
Member

nsajko commented Sep 30, 2024 •
edited
Loading

Explanations for the labels, given that this is your first PR:

Marking as "needs docs" because the doc string should be included into the documentation document (HTML/PDF).

Marking as "needs news" because there's no news entry added, see NEWS.md.

nsajko
nsajko reviewed Sep 30, 2024
View reviewed changes
Comment thread NEWS.md Outdated
nsajko
nsajko reviewed Sep 30, 2024
View reviewed changes
Comment thread NEWS.md Outdated
kuszmaul and others added 2 commits September 30, 2024 15:58
@kuszmaul @nsajko
Update NEWS.md
a3c5fb3 
Co-authored-by: Neven Sajko <s@purelymail.com>
@kuszmaul @nsajko
Update NEWS.md
de8ff48 
Co-authored-by: Neven Sajko <s@purelymail.com>
@nsajko nsajko removed the needs news A NEWS entry is required for this change label Sep 30, 2024
@nsajko
Copy link
Copy Markdown
Member

nsajko commented Sep 30, 2024

The doc strings may be included into the built docs by adding the lines for the new types around here:

julia/doc/src/base/multi-threading.md

Line 66 in bb25910

Base.Threads.SpinLock

nsajko
nsajko reviewed Sep 30, 2024
View reviewed changes
Comment thread test/threads_exec.jl
end
end

if threadpoolsize() > 1
Copy link
Copy Markdown
Member

@nsajko nsajko Sep 30, 2024

Choose a reason for hiding this comment

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

AFAIK it's a good idea to wrap new tests into a @testset.

Copy link
Copy Markdown
Contributor Author

@kuszmaul kuszmaul Oct 1, 2024

Choose a reason for hiding this comment

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

done

@topolarity
Copy link
Copy Markdown
Member

topolarity commented Sep 30, 2024

Why introduce a new PaddedSpinLock rather than improve the implementation of SpinLock?

@oscardssmith
Copy link
Copy Markdown
Member

oscardssmith commented Oct 1, 2024

If you are including the SpinLock in a struct, you often will want to use the other fields of the struct as padding.

@kuszmaul
Copy link
Copy Markdown
Contributor Author

kuszmaul commented Oct 1, 2024 •
edited
Loading

Why introduce a new PaddedSpinLock rather than improve the implementation of SpinLock?

The unpadded SpinLock may be useful in some situations: It's quite a bit smaller than the padded one. I could imagine, for example, someone putting a SpinLock on every vertex of a graph, and being sensitive to the size of the SpinLock. In that case, a little false sharing might be OK. I didn't want to break anyone's code, so I made a new lock..

@kuszmaul
Copy link
Copy Markdown
Contributor Author

kuszmaul commented Oct 1, 2024

If you are including the SpinLock in a struct, you often will want to use the other fields of the struct as padding.

I'm not sure whether that's true.. As I understand it (and I could be wrong) a SpInLock is a mutable struct, so when you include it in another struct, the other fields of the containing struct don't act as padding. All that's stored in the containing struct is a pointer to the SpinLock, and if two such SpinLock's end up on the same cache line, you'll get false sharing. This kind of thing is more likely when using, for example, JEMalloc than some other malloc implementations, since JEMalloc will pack 8-byte objects together on the same cacheline or page.

@nsajko
Copy link
Copy Markdown
Member

nsajko commented Oct 1, 2024

Probably want to add a public statement for the new types. Around here:

julia/base/locks-mt.jl

Line 5 in 32ad9e6

export SpinLock

@kuszmaul
Copy link
Copy Markdown
Contributor Author

kuszmaul commented Oct 1, 2024

Thanks for the various suggestions and the help with process.

nickrobinson251
nickrobinson251 reviewed Oct 3, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@nickrobinson251 nickrobinson251 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

a small docs suggestion

remember to add the new type to the online documentation by adding it here:

julia/doc/src/base/multi-threading.md

Line 66 in bb25910

Base.Threads.SpinLock

and either mark it public or export it

Comment thread base/locks-mt.jl
Comment thread base/locks-mt.jl
@gbaraldi
Copy link
Copy Markdown
Member

gbaraldi commented Oct 22, 2024

Pending review comments LGTM!

@gbaraldi gbaraldi added merge me PR is reviewed. Merge when all tests are passing and removed needs docs Documentation for this change is required labels Nov 11, 2024
@ViralBShah
Copy link
Copy Markdown
Member

ViralBShah commented Nov 12, 2024

The test failures look real.

@ViralBShah ViralBShah removed the merge me PR is reviewed. Merge when all tests are passing label Nov 12, 2024
adienes
adienes reviewed May 5, 2025
View reviewed changes
Comment thread test/threads_exec.jl Outdated
@adienes adienes added the performance Must go faster label May 5, 2025
ViralBShah and others added 2 commits May 5, 2025 11:19
@ViralBShah @adienes
Update test/threads_exec.jl
84dedc1 
Co-authored-by: adienes <51664769+adienes@users.noreply.github.com>
@ViralBShah
Merge branch 'master' into master
03e4c11
@adienes adienes added the merge me PR is reviewed. Merge when all tests are passing label May 11, 2025
@IanButterworth IanButterworth merged commit 1b1b5d5 into JuliaLang:master May 11, 2025
8 checks passed
@ViralBShah
Copy link
Copy Markdown
Member

ViralBShah commented May 11, 2025

Should this be backported?

@oscardssmith
Copy link
Copy Markdown
Member

oscardssmith commented May 11, 2025

no. it's a feature

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label May 12, 2025
charleskawczynski pushed a commit to charleskawczynski/julia that referenced this pull request May 12, 2025
@kuszmaul @charleskawczynski
Implement PaddedSpinLock, which avoids false sharing. (JuliaLang#55944
eb36592 
)
@KristofferC KristofferC mentioned this pull request Dec 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adienes adienes adienes left review comments

@gbaraldi gbaraldi gbaraldi approved these changes

+2 more reviewers

@nsajko nsajko nsajko left review comments

@nickrobinson251 nickrobinson251 nickrobinson251 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature Indicates new feature / enhancement requests parallelism Parallel or distributed computation performance Must go faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SpinLocks suffer from false sharing

10 participants

@kuszmaul @nsajko @topolarity @oscardssmith @gbaraldi @ViralBShah @nickrobinson251 @adienes @IanButterworth @DilumAluthge