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

Implement PaddedSpinLock, which avoids false sharing. #55944

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

kuszmaul
Copy link

@kuszmaul kuszmaul commented Sep 30, 2024

Implemented PaddedSpinLock to help avoid false sharing.

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

Fixes #55942.

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
base/locks-mt.jl Outdated Show resolved Hide resolved
@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
Contributor

nsajko commented Sep 30, 2024

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.

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
kuszmaul and others added 2 commits September 30, 2024 15:58
Co-authored-by: Neven Sajko <[email protected]>
Co-authored-by: Neven Sajko <[email protected]>
@nsajko nsajko removed the needs news A NEWS entry is required for this change label Sep 30, 2024
@nsajko
Copy link
Contributor

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:

Base.Threads.SpinLock

@@ -53,6 +53,26 @@ if threadpoolsize() > 1
end
end

if threadpoolsize() > 1
Copy link
Contributor

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
Author

Choose a reason for hiding this comment

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

done

@topolarity
Copy link
Member

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

@oscardssmith
Copy link
Member

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
Author

kuszmaul commented Oct 1, 2024

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
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
Contributor

nsajko commented Oct 1, 2024

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

export SpinLock

@kuszmaul
Copy link
Author

kuszmaul commented Oct 1, 2024

Thanks for the various suggestions and the help with process.

Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

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:

Base.Threads.SpinLock

and either mark it public or export it

base/locks-mt.jl Show resolved Hide resolved
base/locks-mt.jl Show resolved Hide resolved
@gbaraldi
Copy link
Member

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
Member

The test failures look real.

@ViralBShah ViralBShah removed the merge me PR is reviewed. Merge when all tests are passing label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests parallelism Parallel or distributed computation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SpinLocks suffer from false sharing
7 participants