From da1b26a52ea9976a446883a836e6a7458c0c4212 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sat, 30 Mar 2019 06:43:16 -0300 Subject: [PATCH] adopt raw-id --- src/interned.rs | 94 +++++++++++---------------------------- src/lib.rs | 2 + src/raw_id.rs | 103 +++++++++++++++++++++++++++++++++++++++++++ tests/gc/interned.rs | 12 ++--- tests/interned.rs | 12 ++--- 5 files changed, 144 insertions(+), 79 deletions(-) create mode 100644 src/raw_id.rs diff --git a/src/interned.rs b/src/interned.rs index b6bc9040..ff387367 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -3,6 +3,7 @@ use crate::plumbing::CycleDetected; use crate::plumbing::HasQueryGroup; use crate::plumbing::QueryStorageMassOps; use crate::plumbing::QueryStorageOps; +use crate::raw_id::RawId; use crate::runtime::ChangedAt; use crate::runtime::Revision; use crate::runtime::StampedValue; @@ -46,7 +47,7 @@ where struct InternTables { /// Map from the key to the corresponding intern-index. - map: FxHashMap, + map: FxHashMap, /// For each valid intern-index, stores the interned value. When /// an interned value is GC'd, the entry is set to @@ -54,7 +55,7 @@ struct InternTables { values: Vec>, /// Index of the first free intern-index, if any. - first_free: Option, + first_free: Option, } /// Trait implemented for the "key" that results from a @@ -62,63 +63,22 @@ struct InternTables { /// "newtype"'d `u32`. pub trait InternKey { /// Create an instance of the intern-key from a `u32` value. - fn from_u32(v: u32) -> Self; + fn from_raw_id(v: RawId) -> Self; /// Extract the `u32` with which the intern-key was created. - fn as_u32(&self) -> u32; + fn as_raw_id(&self) -> RawId; } -impl InternKey for u32 { - fn from_u32(v: u32) -> Self { +impl InternKey for RawId { + fn from_raw_id(v: RawId) -> RawId { v } - fn as_u32(&self) -> u32 { + fn as_raw_id(&self) -> RawId { *self } } -impl InternKey for usize { - fn from_u32(v: u32) -> Self { - v as usize - } - - fn as_u32(&self) -> u32 { - assert!(*self <= (std::u32::MAX as usize)); - *self as u32 - } -} - -/// Newtype indicating an index into the intern table. -/// -/// NB. In some cases, `InternIndex` values come directly from the -/// user and hence they are not 'trusted' to be valid or in-bounds. -#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] -struct InternIndex { - index: u32, -} - -impl InternIndex { - fn index(self) -> usize { - self.index as usize - } -} - -impl From for InternIndex { - fn from(v: usize) -> Self { - InternIndex { index: v.as_u32() } - } -} - -impl From<&T> for InternIndex -where - T: InternKey, -{ - fn from(v: &T) -> Self { - InternIndex { index: v.as_u32() } - } -} - enum InternValue { /// The value has not been gc'd. Present { @@ -136,7 +96,7 @@ enum InternValue { }, /// Free-list -- the index is the next - Free { next: Option }, + Free { next: Option }, } impl std::panic::RefUnwindSafe for InternedStorage @@ -205,7 +165,7 @@ where Q::Value: InternKey, DB: Database, { - fn intern_index(&self, db: &DB, key: &Q::Key) -> StampedValue { + fn intern_index(&self, db: &DB, key: &Q::Key) -> StampedValue { if let Some(i) = self.intern_check(db, key) { return i; } @@ -222,7 +182,7 @@ where // Somebody inserted this key while we were waiting // for the write lock. let index = *entry.get(); - match &tables.values[index.index()] { + match &tables.values[index.as_usize()] { InternValue::Present { value, interned_at, @@ -248,7 +208,7 @@ where let index = match tables.first_free { None => { - let index = InternIndex::from(tables.values.len()); + let index = RawId::from(tables.values.len()); tables.values.push(InternValue::Present { value: owned_key2, interned_at: revision_now, @@ -258,7 +218,7 @@ where } Some(i) => { - let next_free = match &tables.values[i.index()] { + let next_free = match &tables.values[i.as_usize()] { InternValue::Free { next } => *next, InternValue::Present { value, .. } => { panic!( @@ -268,7 +228,7 @@ where } }; - tables.values[i.index()] = InternValue::Present { + tables.values[i.as_usize()] = InternValue::Present { value: owned_key2, interned_at: revision_now, accessed_at: revision_now, @@ -289,14 +249,14 @@ where } } - fn intern_check(&self, db: &DB, key: &Q::Key) -> Option> { + fn intern_check(&self, db: &DB, key: &Q::Key) -> Option> { let revision_now = db.salsa_runtime().current_revision(); // First, { let tables = self.tables.read(); let &index = tables.map.get(key)?; - match &tables.values[index.index()] { + match &tables.values[index.as_usize()] { InternValue::Present { interned_at, accessed_at, @@ -325,7 +285,7 @@ where // Next, let mut tables = self.tables.write(); let &index = tables.map.get(key)?; - match &mut tables.values[index.index()] { + match &mut tables.values[index.as_usize()] { InternValue::Present { interned_at, accessed_at, @@ -356,10 +316,10 @@ where fn lookup_value( &self, db: &DB, - index: InternIndex, + index: RawId, op: impl FnOnce(&Q::Key) -> R, ) -> StampedValue { - let index = index.index(); + let index = index.as_usize(); let revision_now = db.salsa_runtime().current_revision(); { @@ -439,7 +399,7 @@ where db.salsa_runtime() .report_query_read(database_key, changed_at); - Ok(::from_u32(value.index)) + Ok(::from_raw_id(value)) } fn maybe_changed_since( @@ -470,9 +430,7 @@ where tables .map .iter() - .map(|(key, index)| { - TableEntry::new(key.clone(), Some(::from_u32(index.index))) - }) + .map(|(key, index)| TableEntry::new(key.clone(), Some(::from_raw_id(*index)))) .collect() } } @@ -507,7 +465,7 @@ where // revision don't have this problem. Anything // dependent on them would regard itself as dirty if // they are removed and also be forced to re-execute. - DiscardIf::Always | DiscardIf::Outdated => match values[intern_index.index()] { + DiscardIf::Always | DiscardIf::Outdated => match values[intern_index.as_usize()] { InternValue::Present { accessed_at, .. } => accessed_at < revision_now, InternValue::Free { .. } => { @@ -520,7 +478,7 @@ where }; if discard { - values[intern_index.index()] = InternValue::Free { next: *first_free }; + values[intern_index.as_usize()] = InternValue::Free { next: *first_free }; *first_free = Some(*intern_index); } @@ -551,7 +509,7 @@ where key: &Q::Key, database_key: &DB::DatabaseKey, ) -> Result { - let index = InternIndex::from(key); + let index = key.as_raw_id(); let group_storage = >::group_storage(db); let interned_storage = IQ::query_storage(group_storage); @@ -571,7 +529,7 @@ where key: &Q::Key, _database_key: &DB::DatabaseKey, ) -> bool { - let index = InternIndex::from(key); + let index = key.as_raw_id(); // NB. This will **panic** if `key` has been removed from the // map, whereas you might expect it to return true in that @@ -617,7 +575,7 @@ where tables .map .iter() - .map(|(key, index)| TableEntry::new(::from_u32(index.index), Some(key.clone()))) + .map(|(key, index)| TableEntry::new(::from_raw_id(*index), Some(key.clone()))) .collect() } } diff --git a/src/lib.rs b/src/lib.rs index 8fc1593d..33c5b28e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,6 +11,7 @@ mod derived; mod input; mod interned; +mod raw_id; mod runtime; pub mod debug; @@ -28,6 +29,7 @@ use std::fmt::{self, Debug}; use std::hash::Hash; pub use crate::interned::InternKey; +pub use crate::raw_id::RawId; pub use crate::runtime::Runtime; pub use crate::runtime::RuntimeId; diff --git a/src/raw_id.rs b/src/raw_id.rs new file mode 100644 index 00000000..d0238e5c --- /dev/null +++ b/src/raw_id.rs @@ -0,0 +1,103 @@ +use std::fmt; + +/// The "raw-id" is used for interned keys in salsa. Typically, it is +/// wrapped in a type of your own devising. For more information about +/// interned keys, see [the interned key RFC][rfc]. +/// +/// # Creating a `RawId` +// +/// RawId values can be constructed using the `From` impls, +/// which are implemented for `u32` and `usize`: +/// +/// ``` +/// # use salsa::RawId; +/// let raw_id = RawId::from(22_u32); +/// ``` +/// +/// You can then convert back to a `u32` like so: +/// +/// ``` +/// # use salsa::RawId; +/// # let raw_id = RawId::from(22_u32); +/// let value = u32::from(raw_id); +/// assert_eq!(value, 22); +/// ``` +/// +/// ## Illegal values +/// +/// Be warned, however, that `RawId` values cannot be created from +/// *arbitrary* values -- in particular large values greater than +/// `RawId::MAX` will panic. Those large values are reserved so that +/// the Rust compiler can use them as sentinel values, which means +/// that (for example) `Option` is represented in a single +/// word. +/// +/// ```should_panic +/// # use salsa::RawId; +/// RawId::from(RawId::MAX); +/// ``` +/// +/// [rfc]: https://github.com/salsa-rs/salsa-rfcs/pull/2 +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct RawId { + value: u32, +} + +impl RawId { + /// The maximum allowed `RawId`. This value can grow between + /// releases without affecting semver. + pub const MAX: u32 = 0xFFFF_FF00; + + unsafe fn new_unchecked(value: u32) -> Self { + debug_assert!(value < RawId::MAX); + RawId { value } + } + + /// Convert this raw-id into a u32 value. + pub fn as_u32(self) -> u32 { + self.value + } + + /// Convert this raw-id into a usize value. + pub fn as_usize(self) -> usize { + self.value as usize + } +} + +impl From for u32 { + fn from(raw: RawId) -> u32 { + raw.value + } +} + +impl From for usize { + fn from(raw: RawId) -> usize { + raw.value as usize + } +} + +impl From for RawId { + fn from(id: u32) -> RawId { + assert!(id < RawId::MAX); + unsafe { RawId::new_unchecked(id) } + } +} + +impl From for RawId { + fn from(id: usize) -> RawId { + assert!(id < (RawId::MAX as usize)); + unsafe { RawId::new_unchecked(id as u32) } + } +} + +impl fmt::Debug for RawId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.value.fmt(f) + } +} + +impl fmt::Display for RawId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.value.fmt(f) + } +} diff --git a/tests/gc/interned.rs b/tests/gc/interned.rs index b47d5878..79ad4d16 100644 --- a/tests/gc/interned.rs +++ b/tests/gc/interned.rs @@ -1,5 +1,5 @@ use crate::db; -use salsa::{Database, SweepStrategy}; +use salsa::{Database, RawId, SweepStrategy}; /// Query group for tests for how interned keys interact with GC. #[salsa::query_group(Intern)] @@ -10,20 +10,20 @@ pub(crate) trait InternDatabase { /// Underlying interning query. #[salsa::interned] - fn intern_str(&self, x: &'static str) -> u32; + fn intern_str(&self, x: &'static str) -> RawId; /// This just executes the intern query and returns the result. - fn repeat_intern1(&self, x: &'static str) -> u32; + fn repeat_intern1(&self, x: &'static str) -> RawId; /// Same as `repeat_intern1`. =) - fn repeat_intern2(&self, x: &'static str) -> u32; + fn repeat_intern2(&self, x: &'static str) -> RawId; } -fn repeat_intern1(db: &impl InternDatabase, x: &'static str) -> u32 { +fn repeat_intern1(db: &impl InternDatabase, x: &'static str) -> RawId { db.intern_str(x) } -fn repeat_intern2(db: &impl InternDatabase, x: &'static str) -> u32 { +fn repeat_intern2(db: &impl InternDatabase, x: &'static str) -> RawId { db.intern_str(x) } diff --git a/tests/interned.rs b/tests/interned.rs index 4d654d90..89c624cf 100644 --- a/tests/interned.rs +++ b/tests/interned.rs @@ -1,5 +1,7 @@ //! Test that you can implement a query using a `dyn Trait` setup. +use salsa::RawId; + #[salsa::database(InternStorage)] #[derive(Default)] struct Database { @@ -23,24 +25,24 @@ impl salsa::ParallelDatabase for Database { #[salsa::query_group(InternStorage)] trait Intern { #[salsa::interned] - fn intern1(&self, x: String) -> u32; + fn intern1(&self, x: String) -> RawId; #[salsa::interned] - fn intern2(&self, x: String, y: String) -> u32; + fn intern2(&self, x: String, y: String) -> RawId; #[salsa::interned] fn intern_key(&self, x: String) -> InternKey; } #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] -pub struct InternKey(u32); +pub struct InternKey(RawId); impl salsa::InternKey for InternKey { - fn from_u32(v: u32) -> Self { + fn from_raw_id(v: RawId) -> Self { InternKey(v) } - fn as_u32(&self) -> u32 { + fn as_raw_id(&self) -> RawId { self.0 } }