Skip to content

Commit

Permalink
refactor(core): small improvements
Browse files Browse the repository at this point in the history
* remove excessive unsafe code in `MessageVTable::lookup()`
* more accurate unsafe comments
* remove obsolete hack `Message::_touch()`
* `ResponseToken::forget()` consume `self`
* avoid needless pointer casts
  • Loading branch information
loyd committed Jun 27, 2024
1 parent 8d157cd commit 720778b
Show file tree
Hide file tree
Showing 10 changed files with 40 additions and 57 deletions.
9 changes: 6 additions & 3 deletions elfo-core/src/envelope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ assert_eq_size!(Envelope, usize);
// TODO: the current size (on x86-64) is 64 bytes, but it can be reduced.
// And... it should be reduced once `TraceId` is extended to 16 bytes.
pub(crate) struct EnvelopeHeader {
/// See [`mailbox.rs`] for more details.
/// See `mailbox.rs` for more details.
pub(crate) link: mailbox::Link,
created_time: Instant, // Now used also as a sent time.
trace_id: TraceId,
Expand Down Expand Up @@ -237,7 +237,7 @@ impl Envelope {
let message_offset = self.header().message_offset;

// SAFETY: `message_offset` refers to the same allocation object.
let ptr = unsafe { self.0.as_ptr().cast::<u8>().add(message_offset as usize) };
let ptr = unsafe { self.0.as_ptr().byte_add(message_offset as usize) };

// SAFETY: `envelope_repr_layout()` guarantees that `ptr` is valid.
unsafe { NonNull::new_unchecked(ptr.cast()) }
Expand Down Expand Up @@ -271,7 +271,10 @@ impl Envelope {
// SAFETY: `self` is properly initialized.
let header = unsafe { self.0.as_mut() };

if let MessageKind::RequestAny(token) | MessageKind::RequestAll(token) = &mut header.kind {
let fake_kind = MessageKind::Regular { sender: Addr::NULL };
let kind = mem::replace(&mut header.kind, fake_kind);

if let MessageKind::RequestAny(token) | MessageKind::RequestAll(token) = kind {
// FIXME: probably invalid for ALL requests, need to decrement remainder.
// REVIEW: DO NOT forget check & fix it before merging.
token.forget();
Expand Down
2 changes: 1 addition & 1 deletion elfo-core/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl ExecResult for () {
impl ExecResult for never::Never {
#[allow(private_interfaces)]
fn unify(self) -> Result<(), BoxedError> {
unreachable!()
self
}
}

Expand Down
2 changes: 1 addition & 1 deletion elfo-core/src/mailbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ impl Mailbox {
pub(crate) async fn recv(&self) -> RecvResult {
loop {
// TODO: it should be possible to use `dequeue_unchecked()` here.
// Preliminarily, we should guarantee that `recv()` can be called only
// Preliminarily, we should guarantee that it can be called only
// by one consumer. However, it's not enough to create a dedicated
// `MailboxConsumer` because users can steal `Context` to another
// task/thread and create a race with the `drop_all()` method.
Expand Down
11 changes: 1 addition & 10 deletions elfo-core/src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,6 @@ pub trait Message:
Self::_type_id() == type_id
}

// Called in `_read()` and `_write()` to avoid
// * [rust#47384](https://github.com/rust-lang/rust/issues/47384)
// * [rust#99721](https://github.com/rust-lang/rust/issues/99721)
#[doc(hidden)]
fn _touch(&self);

#[doc(hidden)]
#[inline(always)]
fn _into_any(self) -> AnyMessage {
Expand Down Expand Up @@ -122,9 +116,7 @@ pub trait Message:
#[inline(always)]
unsafe fn _read(ptr: NonNull<MessageRepr>) -> Self {
let data_ref = &ptr.cast::<MessageRepr<Self>>().as_ref().data;
let data = ptr::read(data_ref);
data._touch();
data
ptr::read(data_ref)
}

/// # Safety
Expand All @@ -137,7 +129,6 @@ pub trait Message:
#[doc(hidden)]
#[inline(always)]
unsafe fn _write(self, ptr: NonNull<MessageRepr>) {
self._touch();
let repr = MessageRepr::new(self);
ptr::write(ptr.cast::<MessageRepr<Self>>().as_ptr(), repr);
}
Expand Down
9 changes: 2 additions & 7 deletions elfo-core/src/message/any.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,6 @@ impl Message for AnyMessage {
true
}

#[inline(always)]
fn _touch(&self) {}

#[inline(always)]
fn _into_any(self) -> AnyMessage {
self
Expand Down Expand Up @@ -431,16 +428,14 @@ impl<'a> AnyMessageRef<'a> {
pub fn downcast_ref<M: Message>(&self) -> Option<&'a M> {
let ret = self.inner.downcast_ref();

// SAFETY: we produce lifetime bound to the original one.
// Note: semantically it's shortening not extending.
// SAFETY: we produce lifetime bound to the original `AnyMessage` or `Envelope`.
unsafe { mem::transmute::<Option<&M>, Option<&'a M>>(ret) }
}

pub(crate) unsafe fn downcast_ref_unchecked<M: Message>(&self) -> &'a M {
let ret = self.inner.downcast_ref_unchecked();

// SAFETY: we produce lifetime bound to the original one.
// Note: semantically it's shortening not extending.
// SAFETY: we produce lifetime bound to the original `AnyMessage` or `Envelope`.
unsafe { mem::transmute::<&M, &'a M>(ret) }
}
}
Expand Down
53 changes: 25 additions & 28 deletions elfo-core/src/message/repr.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use std::{
alloc, fmt,
alloc,
borrow::Borrow,
fmt,
ptr::{self, NonNull},
};

Expand Down Expand Up @@ -69,8 +71,9 @@ impl PartialEq for MessageTypeId {
#[repr(C)]
// It's `pub` only because used in private methods of `AnyMessage`.
// Actually, it's not reexported at all.
#[derive(Clone)]
#[doc(hidden)]
pub struct MessageRepr<M = ()> {
pub struct MessageRepr<M = Erased> {
pub(crate) vtable: &'static MessageVTable,
pub(crate) data: M,
}
Expand All @@ -88,18 +91,13 @@ impl<M: Message> MessageRepr<M> {
}
}

// Actually, it's not reexported at all.
#[doc(hidden)]
pub struct Erased;

// Protection against footgun.
assert_not_impl_any!(MessageRepr: Clone);

impl<M: Message> Clone for MessageRepr<M> {
fn clone(&self) -> Self {
Self {
vtable: self.vtable,
data: self.data.clone(),
}
}
}

// === MessageVTable ===

/// Message Virtual Table.
Expand Down Expand Up @@ -269,28 +267,27 @@ mod vtablefns {
#[linkme::distributed_slice]
pub static MESSAGE_VTABLES_LIST: [&'static MessageVTable] = [..];

static MESSAGE_VTABLES_MAP: Lazy<FxHashMap<(&'static str, &'static str), &'static MessageVTable>> =
Lazy::new(|| {
MESSAGE_VTABLES_LIST
.iter()
.map(|vtable| ((vtable.protocol, vtable.name), *vtable))
.collect()
});
#[derive(PartialEq, Eq, Hash)]
pub struct Signature([&'static str; 2]); // [protocol, name]

impl<'a> Borrow<[&'a str; 2]> for Signature {
fn borrow(&self) -> &[&'a str; 2] {
&self.0
}
}

static MESSAGE_VTABLES_MAP: Lazy<FxHashMap<Signature, &'static MessageVTable>> = Lazy::new(|| {
MESSAGE_VTABLES_LIST
.iter()
.map(|vtable| (Signature([vtable.protocol, vtable.name]), *vtable))
.collect()
});

impl MessageVTable {
/// Finds a vtable by protocol and name.
/// Used for deserialization of `AnyMessage` and in networking.
pub(crate) fn lookup(protocol: &str, name: &str) -> Option<&'static Self> {
// Extend lifetimes to static in order to get `(&'static str, &'static str)`.
// SAFETY: this pair doesn't overlive the function.
let (protocol, name) = unsafe {
(
std::mem::transmute::<&str, &'static str>(protocol),
std::mem::transmute::<&str, &'static str>(name),
)
};

MESSAGE_VTABLES_MAP.get(&(protocol, name)).copied()
MESSAGE_VTABLES_MAP.get(&[protocol, name]).copied()
}
}

Expand Down
2 changes: 1 addition & 1 deletion elfo-core/src/request_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ impl ResponseToken {

#[doc(hidden)]
#[inline]
pub fn forget(&mut self) {
pub fn forget(mut self) {
self.data = None;
}

Expand Down
2 changes: 1 addition & 1 deletion elfo-logger/src/line_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use super::line_buffer::LineBuffer;
/// This is needed due to rust's closure lifetimes binding limitation. Consider,
/// for example, the following implementation:
/// ```no_compile
/// triat LineFactory<'a>: FnOnce(&'a LineBuffer) -> Self::Line {
/// trait LineFactory<'a>: FnOnce(&'a LineBuffer) -> Self::Line {
/// type Line: Line + 'a;
/// }
/// ```
Expand Down
3 changes: 0 additions & 3 deletions elfo-macros-impl/src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,6 @@ pub fn message_impl(
fn _vtable(&self) -> &'static #internal::MessageVTable {
VTABLE
}

#[inline(never)]
fn _touch(&self) {}
}

#[#internal::linkme::distributed_slice(#internal::MESSAGE_VTABLES_LIST)]
Expand Down
4 changes: 2 additions & 2 deletions elfo-network/src/worker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ fn make_network_envelope(
(sender, trace_id, payload, token)
}
// Response
(Ok(envelope), Some(mut token)) => {
(Ok(envelope), Some(token)) => {
let sender = envelope.sender();
let trace_id = envelope.trace_id();
let (message, kind) = envelope.unpack::<AnyMessage>().expect("impossible");
Expand All @@ -323,7 +323,7 @@ fn make_network_envelope(
(sender, trace_id, payload, None)
}
// Failed/Ignored Response
(Err(err), Some(mut token)) => {
(Err(err), Some(token)) => {
let sender = Addr::NULL;
let trace_id = token.trace_id();

Expand Down

0 comments on commit 720778b

Please sign in to comment.