Skip to content

Add cache line size padding to RWLock #117519

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

Closed
wants to merge 2 commits into from

Conversation

mj10021
Copy link
Contributor

@mj10021 mj10021 commented Nov 2, 2023

Fixing issue #117470, this PR just copies the cfg_attr repr from the cache padding crate to stop RWLocks from sharing cache lines.

@rustbot
Copy link
Collaborator

rustbot commented Nov 2, 2023

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 2, 2023
@mj10021
Copy link
Contributor Author

mj10021 commented Nov 2, 2023

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@bors
Copy link
Collaborator

bors commented Nov 2, 2023

@mj10021: 🔑 Insufficient privileges: not in try users

@Kobzol
Copy link
Member

Kobzol commented Nov 2, 2023

@bors try @rust-timer queue

@bors
Copy link
Collaborator

bors commented Nov 2, 2023

⌛ Trying commit 1e43257 with merge e08cb3e...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2023
Add cache line size padding to RWLock

Fixing issue rust-lang#117470, this PR just copies the `cfg_attr` `repr` from the cache padding crate to stop RWLocks from sharing cache lines.
@bors
Copy link
Collaborator

bors commented Nov 2, 2023

☀️ Try build successful - checks-actions
Build commit: e08cb3e (e08cb3ef876cf8045096a682db4e7fe0baafc4db)

@cuviper
Copy link
Member

cuviper commented Nov 2, 2023

We have a CachePadded<T> in mpmc::utils that also does this, which you could wrap around the inner field. That should keep it out of the docs too.

@cuviper
Copy link
Member

cuviper commented Nov 2, 2023

Alternatively, it might make more sense for the original reporter to wrap their own CachePadded<RwLock<T>> for their use case, without inflating the size for all std users.

@mj10021
Copy link
Contributor Author

mj10021 commented Nov 3, 2023

Alternatively, it might make more sense for the original reporter to wrap their own CachePadded<RwLock<T>> for their use case, without inflating the size for all std users.

Yeah that makes sense to me since its a pretty simple optimization on a case by case basis. Would it make sense to mention in the RWLock docs that padding to cache line size can be a known speed optomization?

@quininer
Copy link
Contributor

quininer commented Nov 6, 2023

Going one step further, maybe the std should provide the CachePadded? this is a very commonly used tool in concurrent programming.

Copy link

@pingzhaozz pingzhaozz left a comment

Choose a reason for hiding this comment

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

repr(align(n)) doesn't work here. Inside the structure it requires something like:

struct RwLock<T> {
   inner : CachePadded<RwLock>,
   ...
}

But std seems not support such kind yet.

@Noratrieb
Copy link
Member

As others have already commented, making these locks this big for everyone seems like a big pessimization if the locks aren't in a really hot path next to each other.
I think it makes sense to close this and suggest moving forwards with adding the cache padding outside the lock, either with a type in std with the ACP or not. Maybe it would make sense to also mention this in the documentation.
Thank you for the PR anyways!

@Noratrieb Noratrieb closed this Jan 21, 2024
@mj10021 mj10021 deleted the issue-117470-fix branch November 1, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants