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.

Sign up for GitHub

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

Jump to bottom

CachePadded: update alignment based on golang's CacheLinePadSize #636

Merged
merged 1 commit into from
Jan 3, 2021
Merged

CachePadded: update alignment based on golang's CacheLinePadSize #636

merged 1 commit into from
Jan 3, 2021

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Jan 2, 2021

This updates alignment of CachePadded on the following architectures based on golang's CacheLinePadSize.

On x86_64 and aarch64, use 128-byte alignment as before. See #331 and #499 for reasons.
On all others, use 64-byte alignment as before.

Closes #427

cc @stjepang (when this merged, I'll send a same patch to smol-rs/cache-padded)
FYI @yoshuawuyts @cuviper (because you all were discussing about adding CachePadded to the standard library)

@taiki-e taiki-e requested review from jeehoonkang and Vtec234 January 2, 2021 16:09
jeehoonkang
jeehoonkang approved these changes Jan 3, 2021
View reviewed changes
Copy link
Contributor

@jeehoonkang jeehoonkang left a comment

Choose a reason for hiding this comment

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

I like this change, but I'm not an expert on this and I'd like one more +1.

yoshuawuyts
yoshuawuyts approved these changes Jan 3, 2021
View reviewed changes
Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Oh amazing; yes!

Adding in the references is 💯

@jeehoonkang
Copy link
Contributor

bors r+

Vtec234
Vtec234 approved these changes Jan 3, 2021
View reviewed changes
Copy link
Member

@Vtec234 Vtec234 left a comment

Choose a reason for hiding this comment

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

There is still the issue of some AMD CPUs fetching just 64 (maybe? I'm not an expert), but that would have to be a runtime check since we don't compile for specific CPUs so LGTM.

@bors
Copy link
Contributor

bors bot commented Jan 3, 2021

Build succeeded:

@bors bors bot merged commit 92ca2da into crossbeam-rs:master Jan 3, 2021
@novacrazy
Copy link

Indeed, all AMD CPUs still use a 64-byte cacheline size, so this implementation will use up two lines on AMD.

Previously, I had used a build script to detect target-cpu and use that to control my own CachePadded, and in the past year there has been zero progress on better target introspection in Rust, so oh well.

@taiki-e taiki-e deleted the cache-padded branch January 4, 2021 04:05
@taiki-e
Copy link
Member Author

taiki-e commented Jan 4, 2021 •
edited
Loading

We may want to determine the principle regarding which cache line size CachePadded chooses in architectures where the actual cache line size is implementation-defined. (Currently, on x86_64 and aarch64, we refer to implementation that uses the largest cache line size.)

Previously, I had used a build script to detect target-cpu and use that to control my own CachePadded, and in the past year there has been zero progress on better target introspection in Rust, so oh well.

I think we might be able to add an option (--cfg) to detect native CPU cache line size using a build script. (It is never enabled by default for the same reason that -C target-cpu=native is optional.) But for now, it may be better to implement it as a separate crate.

@cuviper
Copy link
Contributor

cuviper commented Jan 4, 2021

I think if you start digging that deep, it will be better served by the standard library and compiler intrinsics, where it has the canonical view of the target cpu and features.

@taiki-e taiki-e mentioned this pull request Feb 13, 2021
Merged
bors bot added a commit that referenced this pull request Feb 20, 2021
@bors @taiki-e
Merge #659
d709c89 
659: Prepare for the next release r=taiki-e a=taiki-e

It's been over two months since the previous release. There are some improvements and deprecations in the master branch, and it would be nice to release them. Also, there is no breaking change that needs a major version bump.

Changes:

- crossbeam-epoch 0.9.1 -> 0.9.2
  - Add `Atomic::compare_exchange` and `Atomic::compare_exchange_weak`. (#628)
  - Deprecate `Atomic::compare_and_set` and `Atomic::compare_and_set_weak`. (#628)
  - Make `const_fn` dependency optional. (#611)
  - Add unstable support for `loom`. (#487)
- crossbeam-utils 0.8.1 -> 0.8.2
  - Deprecate `AtomicCell::compare_and_swap`. (#619)
  - Add `Parker::park_deadline`. (#563)
  - Improve implementation of `CachePadded`. (#636)
  - Add unstable support for `loom`. (#487)


Co-authored-by: Taiki Endo <te316e89@gmail.com>
@mgeier mgeier mentioned this pull request Dec 11, 2021
Closed
taiki-e added a commit to smol-rs/cache-padded that referenced this pull request Dec 19, 2021
@taiki-e
CachePadded: update alignment based on golang's CacheLinePadSize
a17da7a 
Refs: crossbeam-rs/crossbeam#636
Closes: #7
taiki-e added a commit to smol-rs/cache-padded that referenced this pull request Dec 19, 2021
@taiki-e
CachePadded: update alignment based on golang's CacheLinePadSize
4595799 
Refs: crossbeam-rs/crossbeam#636
Closes: #7
@wathenjiang wathenjiang mentioned this pull request Jul 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vtec234 Vtec234 Vtec234 approved these changes

@yoshuawuyts yoshuawuyts yoshuawuyts approved these changes

@jeehoonkang jeehoonkang jeehoonkang approved these changes

Assignees
No one assigned
Labels
crossbeam-utils
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

More complete implementation of CachePadded
6 participants
@taiki-e @jeehoonkang @novacrazy @cuviper @yoshuawuyts @Vtec234