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

Use inline assembly in full_fence #71

Merged
merged 1 commit into from
Aug 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ jobs:
matrix:
# When updating this, the reminder to update the minimum supported
# Rust version in Cargo.toml.
rust: ['1.39']
rust: ['1.59']
steps:
- uses: actions/checkout@v3
- name: Install Rust
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ name = "event-listener"
version = "2.5.3"
authors = ["Stjepan Glavina <stjepang@gmail.com>"]
edition = "2018"
rust-version = "1.39"
rust-version = "1.59"
description = "Notify async tasks or threads"
license = "Apache-2.0 OR MIT"
repository = "https://github.com/smol-rs/event-listener"
Expand Down
10 changes: 0 additions & 10 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,16 +110,6 @@ pub mod prelude {
pub use crate::{IntoNotification, Notification};
}

/// 1.39-compatible replacement for `matches!`
macro_rules! matches {
($expr:expr, $($pattern:pat)|+ $(if $guard: expr)?) => {
match $expr {
$($pattern)|+ $(if $guard)? => true,
_ => false,
}
};
}

/// Inner state of [`Event`].
struct Inner<T> {
/// The number of notified entries, or `usize::MAX` if all of them have been notified.
Expand Down
34 changes: 19 additions & 15 deletions src/notify.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! The `Notification` trait for specifying notification.

use crate::sync::atomic::{self, AtomicUsize, Ordering};
use crate::sync::atomic::{self, Ordering};
use core::fmt;

pub(crate) use __private::Internal;
Expand Down Expand Up @@ -555,27 +555,31 @@ impl_for_numeric_types! { usize u8 u16 u32 u64 u128 isize i8 i16 i32 i64 i128 }
/// Equivalent to `atomic::fence(Ordering::SeqCst)`, but in some cases faster.
#[inline]
pub(super) fn full_fence() {
if cfg!(all(
any(target_arch = "x86", target_arch = "x86_64"),
not(miri)
)) {
#[cfg(all(any(target_arch = "x86", target_arch = "x86_64"), not(miri), not(loom)))]
{
use core::{arch::asm, cell::UnsafeCell};
// HACK(stjepang): On x86 architectures there are two different ways of executing
// a `SeqCst` fence.
//
// 1. `atomic::fence(SeqCst)`, which compiles into a `mfence` instruction.
// 2. `_.compare_exchange(_, _, SeqCst, SeqCst)`, which compiles into a `lock cmpxchg` instruction.
// 2. A `lock <op>` instruction.
//
// Both instructions have the effect of a full barrier, but empirical benchmarks have shown
// that the second one is sometimes a bit faster.
//
// The ideal solution here would be to use inline assembly, but we're instead creating a
// temporary atomic variable and compare-and-exchanging its value. No sane compiler to
// x86 platforms is going to optimize this away.
atomic::compiler_fence(Ordering::SeqCst);
let a = AtomicUsize::new(0);
let _ = a.compare_exchange(0, 1, Ordering::SeqCst, Ordering::SeqCst);
atomic::compiler_fence(Ordering::SeqCst);
} else {
let a = UnsafeCell::new(0_usize);
// It is common to use `lock or` here, but when using a local variable, `lock not`, which
// does not change the flag, should be slightly more efficient.
Comment on lines +570 to +571
Copy link
Collaborator Author

@taiki-e taiki-e Aug 13, 2023

Choose a reason for hiding this comment

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

(Note on potential future optimization: I have not benchmarked, but using xchg (lock prefix is implied) here may potentially be faster than lock not. The number of registers used will increase, but the value at a and the value to be written can be uninit. Making the value at a uninit with lock not is fine from a hardware standpoint, but Valgrind will probably be angry about it.)

// Refs: https://www.felixcloutier.com/x86/not
unsafe {
#[cfg(target_pointer_width = "64")]
asm!("lock not qword ptr [{0}]", in(reg) a.get(), options(nostack, preserves_flags));
#[cfg(target_pointer_width = "32")]
asm!("lock not dword ptr [{0:e}]", in(reg) a.get(), options(nostack, preserves_flags));
}
return;
notgull marked this conversation as resolved.
Show resolved Hide resolved
}
#[allow(unreachable_code)]
{
atomic::fence(Ordering::SeqCst);
}
}
Expand Down
2 changes: 1 addition & 1 deletion strategy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ name = "event-listener-strategy"
version = "0.1.0"
edition = "2018"
authors = ["John Nunley <jtnunley01@gmail.com>"]
rust-version = "1.39"
rust-version = "1.59"
description = "Block or poll on event_listener easily"
license = "Apache-2.0 OR MIT"
repository = "https://github.com/smol-rs/event-listener"
Expand Down