From 79f657b462e45cd74445cee976490e45392e1eba Mon Sep 17 00:00:00 2001 From: Sayan Nandan Date: Fri, 25 Jun 2021 09:18:30 +0530 Subject: [PATCH] Add more LLVM specific optimizations Just to reduce LLVM bloat --- server/src/actions/get.rs | 9 ++---- server/src/actions/jget.rs | 9 +++--- server/src/actions/keylen.rs | 9 ++---- server/src/actions/set.rs | 21 +++++------- server/src/actions/strong.rs | 18 ++++------- server/src/actions/update.rs | 21 +++++------- server/src/admin/mksnap.rs | 19 ++++++----- server/src/dbnet/connection.rs | 1 + server/src/main.rs | 1 + server/src/protocol/mod.rs | 39 +++++++++------------- server/src/util.rs | 59 ++++++++++++++++++++++++++++++++++ 11 files changed, 117 insertions(+), 89 deletions(-) create mode 100644 server/src/util.rs diff --git a/server/src/actions/get.rs b/server/src/actions/get.rs index abcb70fe..596bf7c8 100644 --- a/server/src/actions/get.rs +++ b/server/src/actions/get.rs @@ -32,7 +32,6 @@ use crate::protocol::responses; use crate::queryengine::ActionIter; use crate::resp::BytesWrapper; use bytes::Bytes; -use core::hint::unreachable_unchecked; /// Run a `GET` query pub async fn get( @@ -48,14 +47,10 @@ where let res: Option = { let reader = handle.get_ref(); unsafe { - // UNSAFE(@ohsayan): unreachable_unchecked is safe because we've already checked if the action + // UNSAFE(@ohsayan): this is safe because we've already checked if the action // group contains one argument (excluding the action itself) reader - .get( - act.next() - .unwrap_or_else(|| unreachable_unchecked()) - .as_bytes(), - ) + .get(act.next().unsafe_unwrap().as_bytes()) .map(|b| b.get_blob().clone()) } }; diff --git a/server/src/actions/jget.rs b/server/src/actions/jget.rs index 43338ca9..0733549a 100644 --- a/server/src/actions/jget.rs +++ b/server/src/actions/jget.rs @@ -54,9 +54,8 @@ where } mod json { + use crate::util::Unwrappable; use bytes::Bytes; - use std::hint::unreachable_unchecked; - pub struct BuiltJSON(Vec); pub struct JSONBlob(Vec); impl JSONBlob { @@ -79,10 +78,10 @@ mod json { self.0.push(b','); } pub fn finish(mut self) -> BuiltJSON { - *self.0.last_mut().unwrap_or_else(|| unsafe { + *unsafe { // UNSAFE(@ohsayan): There will always be a value corresponding to last_mut - unreachable_unchecked() - }) = b'}'; + self.0.last_mut().unsafe_unwrap() + } = b'}'; BuiltJSON(self.0) } } diff --git a/server/src/actions/keylen.rs b/server/src/actions/keylen.rs index ab8b3726..c9116ac5 100644 --- a/server/src/actions/keylen.rs +++ b/server/src/actions/keylen.rs @@ -27,7 +27,6 @@ use crate::dbnet::connection::prelude::*; use crate::protocol::responses; use crate::queryengine::ActionIter; -use core::hint::unreachable_unchecked; /// Run a `KEYLEN` query /// @@ -45,14 +44,10 @@ where let res: Option = { let reader = handle.get_ref(); unsafe { - // UNSAFE(@ohsayan): unreachable_unchecked() is completely safe as we've already checked + // UNSAFE(@ohsayan): this is completely safe as we've already checked // the number of arguments is one reader - .get( - act.next() - .unwrap_or_else(|| unreachable_unchecked()) - .as_bytes(), - ) + .get(act.next().unsafe_unwrap().as_bytes()) .map(|b| b.get_blob().len()) } }; diff --git a/server/src/actions/set.rs b/server/src/actions/set.rs index 283873f1..4f824716 100644 --- a/server/src/actions/set.rs +++ b/server/src/actions/set.rs @@ -32,7 +32,6 @@ use crate::dbnet::connection::prelude::*; use crate::protocol::responses; use crate::queryengine::ActionIter; use coredb::Data; -use std::hint::unreachable_unchecked; /// Run a `SET` query pub async fn set( @@ -52,18 +51,14 @@ where let writer = handle.get_ref(); // clippy thinks we're doing something complex when we aren't, at all! #[allow(clippy::blocks_in_if_conditions)] - if writer.true_if_insert( - Data::from(act.next().unwrap_or_else(|| unsafe { - // UNSAFE(@ohsayan): This is completely safe as we've already checked - // that there are exactly 2 arguments - unreachable_unchecked() - })), - Data::from(act.next().unwrap_or_else(|| unsafe { - // UNSAFE(@ohsayan): This is completely safe as we've already checked - // that there are exactly 2 arguments - unreachable_unchecked() - })), - ) { + if unsafe { + // UNSAFE(@ohsayan): This is completely safe as we've already checked + // that there are exactly 2 arguments + writer.true_if_insert( + Data::from(act.next().unsafe_unwrap()), + Data::from(act.next().unsafe_unwrap()), + ) + } { Some(true) } else { Some(false) diff --git a/server/src/actions/strong.rs b/server/src/actions/strong.rs index 4b002be7..3812af5e 100644 --- a/server/src/actions/strong.rs +++ b/server/src/actions/strong.rs @@ -137,11 +137,11 @@ where act.into_iter().for_each(|key| { // Since we've already checked that the keys don't exist // We'll tell the compiler to optimize this - let _ = mut_table.remove(key.as_bytes()).unwrap_or_else(|| unsafe { + unsafe { // UNSAFE(@ohsayan): Since all the values exist, all of them will return // some value. Hence, this branch won't ever be reached. Hence, this is safe. - unreachable_unchecked() - }); + let _ = mut_table.remove(key.as_bytes()).unsafe_unwrap(); + } }); } else { failed = Some(true); @@ -196,17 +196,13 @@ where } // Skip the next value that is coming our way, as we don't need it // right now - let _ = key_iter - .next() - .unwrap_or_else(|| unsafe { unreachable_unchecked() }); + unsafe { + let _ = key_iter.next().unsafe_unwrap(); + } } // clippy thinks we're doing something complex when we aren't, at all! #[allow(clippy::blocks_in_if_conditions)] - if !failed.unwrap_or_else(|| unsafe { - // UNSAFE(@ohsayan): This is completely safe as a value is assigned to `failed` - // right in the beginning - unreachable_unchecked() - }) { + if unsafe { !failed.unsafe_unwrap() } { // Since the failed flag is false, none of the keys existed // So we can safely update the keys while let (Some(key), Some(value)) = (act.next(), act.next()) { diff --git a/server/src/actions/update.rs b/server/src/actions/update.rs index b6264758..6334aee4 100644 --- a/server/src/actions/update.rs +++ b/server/src/actions/update.rs @@ -33,7 +33,6 @@ use crate::dbnet::connection::prelude::*; use crate::protocol::responses; use crate::queryengine::ActionIter; use coredb::Data; -use std::hint::unreachable_unchecked; /// Run an `UPDATE` query pub async fn update( @@ -53,18 +52,14 @@ where let writer = handle.get_ref(); // clippy thinks we're doing something complex when we aren't, at all! #[allow(clippy::blocks_in_if_conditions)] - if writer.true_if_update( - Data::from(act.next().unwrap_or_else(|| unsafe { - // UNSAFE(@ohsayan): We've already checked that the action contains exactly - // two arguments (excluding the action itself). So, this branch won't ever be reached - unreachable_unchecked() - })), - Data::from(act.next().unwrap_or_else(|| unsafe { - // UNSAFE(@ohsayan): We've already checked that the action contains exactly - // two arguments (excluding the action itself). So, this branch won't ever be reached - unreachable_unchecked() - })), - ) { + if unsafe { + // UNSAFE(@ohsayan): This is completely safe as we've already checked + // that there are exactly 2 arguments + writer.true_if_update( + Data::from(act.next().unsafe_unwrap()), + Data::from(act.next().unsafe_unwrap()), + ) + } { Some(true) } else { Some(false) diff --git a/server/src/admin/mksnap.rs b/server/src/admin/mksnap.rs index 6880fe16..cc732fe7 100644 --- a/server/src/admin/mksnap.rs +++ b/server/src/admin/mksnap.rs @@ -30,7 +30,6 @@ use crate::diskstore::snapshot::SnapshotEngine; use crate::diskstore::snapshot::DIR_SNAPSHOT; use crate::protocol::responses; use crate::queryengine::ActionIter; -use std::hint::unreachable_unchecked; use std::path::{Component, PathBuf}; /// Create a snapshot @@ -57,22 +56,22 @@ where let mut succeeded = None; let snaphandle = handle.shared.clone(); - let snapstatus = snaphandle.snapcfg.as_ref().unwrap_or_else(|| unsafe { + let snapstatus = unsafe { // UNSAFE(@ohsayan) This is safe as we've already checked // if snapshots are enabled or not with `is_snapshot_enabled` - unreachable_unchecked() - }); + snaphandle.snapcfg.as_ref().unsafe_unwrap() + }; let snapengine = SnapshotEngine::new(snapstatus.max, &handle, None); if snapengine.is_err() { was_engine_error = true; } else if snapstatus.is_busy() { succeeded = None; } else { - let snapengine = snapengine.unwrap_or_else(|_| unsafe { + let snapengine = unsafe { // UNSAFE(@ohsayan) This is safe as we've already checked // if snapshots are enabled or not with `is_snapshot_enabled` - unreachable_unchecked() - }); + snapengine.unsafe_unwrap() + }; succeeded = Some(snapengine); } if was_engine_error { @@ -99,11 +98,11 @@ where } } else if act.len() == 1 { // This means that the user wants to create a 'named' snapshot - let snapname = act.next().unwrap_or_else(|| unsafe { + let snapname = unsafe { // UNSAFE(@ohsayan): We've already checked that the action // contains a second argument, so this can't be reached - unreachable_unchecked() - }); + act.next().unsafe_unwrap() + }; let mut path = PathBuf::from(DIR_SNAPSHOT); path.push("remote"); path.push(snapname.to_owned() + ".snapshot"); diff --git a/server/src/dbnet/connection.rs b/server/src/dbnet/connection.rs index f504227d..6621920b 100644 --- a/server/src/dbnet/connection.rs +++ b/server/src/dbnet/connection.rs @@ -74,6 +74,7 @@ pub mod prelude { //! //! This module is hollow itself, it only re-exports from `dbnet::con` and `tokio::io` pub use super::ProtocolConnectionExt; + pub use crate::util::Unwrappable; pub use tokio::io::{AsyncReadExt, AsyncWriteExt}; } diff --git a/server/src/main.rs b/server/src/main.rs index 73e57e44..7a085c9f 100644 --- a/server/src/main.rs +++ b/server/src/main.rs @@ -59,6 +59,7 @@ mod resp; mod services; #[cfg(test)] mod tests; +mod util; const PATH: &str = ".sky_pid"; diff --git a/server/src/protocol/mod.rs b/server/src/protocol/mod.rs index 5644eeb1..2b9eef7e 100644 --- a/server/src/protocol/mod.rs +++ b/server/src/protocol/mod.rs @@ -38,8 +38,8 @@ mod element; pub mod responses; +use crate::util::Unwrappable; pub use element::Element; -use std::hint::unreachable_unchecked; #[derive(Debug)] /// # Skyhash Deserializer (Parser) @@ -185,13 +185,11 @@ impl<'a> Parser<'a> { // 48 is the ASCII code for 0, and 57 is the ascii code for 9 // so if 0 is given, the subtraction should give 0; similarly // if 9 is given, the subtraction should give us 9! - let curdig: usize = dig - .checked_sub(48) - .unwrap_or_else(|| unsafe { - // UNSAFE(@ohsayan): We already know that dig is an ASCII digit - unreachable_unchecked() - }) - .into(); + let curdig: usize = unsafe { + // UNSAFE(@ohsayan): We already know that dig is an ASCII digit + dig.checked_sub(48).unsafe_unwrap() + } + .into(); // The usize can overflow; check that case let product = match item_usize.checked_mul(10) { Some(not_overflowed) => not_overflowed, @@ -220,13 +218,11 @@ impl<'a> Parser<'a> { // 48 is the ASCII code for 0, and 57 is the ascii code for 9 // so if 0 is given, the subtraction should give 0; similarly // if 9 is given, the subtraction should give us 9! - let curdig: u64 = dig - .checked_sub(48) - .unwrap_or_else(|| unsafe { - // UNSAFE(@ohsayan): We already know that dig is an ASCII digit - unreachable_unchecked() - }) - .into(); + let curdig: u64 = unsafe { + // UNSAFE(@ohsayan): We already know that dig is an ASCII digit + dig.checked_sub(48).unsafe_unwrap() + } + .into(); // Now the entire u64 can overflow, so let's attempt to check it let product = match item_u64.checked_mul(10) { Some(not_overflowed) => not_overflowed, @@ -379,14 +375,11 @@ impl<'a> Parser<'a> { // of the next query // clippy thinks we're doing something complex when we aren't, at all! #[allow(clippy::blocks_in_if_conditions)] - if self - .will_cursor_give_char(b'*', true) - .unwrap_or_else(|_| unsafe { - // UNSAFE(@ohsayan): This will never be the case because we'll always get a result and no error value - // as we've passed true which will yield Ok(true) even if there is no byte ahead - unreachable_unchecked() - }) - { + if unsafe { + // UNSAFE(@ohsayan): This will never be the case because we'll always get a result and no error value + // as we've passed true which will yield Ok(true) even if there is no byte ahead + self.will_cursor_give_char(b'*', true).unsafe_unwrap() + } { Ok((Query::SimpleQuery(single_group), self.cursor)) } else { // the next item isn't the beginning of a query but something else? diff --git a/server/src/util.rs b/server/src/util.rs new file mode 100644 index 00000000..03018888 --- /dev/null +++ b/server/src/util.rs @@ -0,0 +1,59 @@ +/* + * Created on Fri Jun 25 2021 + * + * This file is a part of Skytable + * Skytable (formerly known as TerrabaseDB or Skybase) is a free and open-source + * NoSQL database written by Sayan Nandan ("the Author") with the + * vision to provide flexibility in data modelling without compromising + * on performance, queryability or scalability. + * + * Copyright (c) 2021, Sayan Nandan + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * +*/ + +/// # Unsafe unwrapping +/// +/// This trait provides a method `unsafe_unwrap` that is potentially unsafe and has +/// the ability to **violate multiple safety gurantees** that rust provides. So, +/// if you get `SIGILL`s or `SIGSEGV`s, by using this trait, blame yourself. +pub unsafe trait Unwrappable { + /// Unwrap a _nullable_ (almost) type to get its value while asserting that the value + /// cannot ever be null + /// + /// ## Safety + /// The trait is unsafe, and so is this function. You can wreck potential havoc if you + /// use this heedlessly + /// + unsafe fn unsafe_unwrap(self) -> T; +} + +unsafe impl Unwrappable for Result { + unsafe fn unsafe_unwrap(self) -> T { + match self { + Ok(t) => t, + Err(_) => core::hint::unreachable_unchecked(), + } + } +} + +unsafe impl Unwrappable for Option { + unsafe fn unsafe_unwrap(self) -> T { + match self { + Some(t) => t, + None => core::hint::unreachable_unchecked(), + } + } +}