Skip to content

perf: Reduce memory usage of salsa slots by 8 bytes #17638

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

Merged
merged 1 commit into from
Jul 19, 2024
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
17 changes: 7 additions & 10 deletions crates/salsa/src/derived/slot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use crate::lru::LruNode;
use crate::plumbing::{DatabaseOps, QueryFunction};
use crate::revision::Revision;
use crate::runtime::local_state::ActiveQueryGuard;
use crate::runtime::local_state::QueryInputs;
use crate::runtime::local_state::QueryRevisions;
use crate::runtime::Runtime;
use crate::runtime::RuntimeId;
Expand All @@ -28,8 +27,8 @@ where
key_index: u32,
group_index: u16,
state: RwLock<QueryState<Q>>,
policy: PhantomData<MP>,
lru_index: LruIndex,
policy: PhantomData<MP>,
}

/// Defines the "current state" of query's memoized results.
Expand Down Expand Up @@ -430,7 +429,8 @@ where
tracing::debug!("Slot::invalidate(new_revision = {:?})", new_revision);
match &mut *self.state.write() {
QueryState::Memoized(memo) => {
memo.revisions.inputs = QueryInputs::Untracked;
memo.revisions.untracked = true;
memo.revisions.inputs = None;
memo.revisions.changed_at = new_revision;
Some(memo.revisions.durability)
}
Expand Down Expand Up @@ -746,11 +746,8 @@ where
match &self.revisions.inputs {
// We can't validate values that had untracked inputs; just have to
// re-execute.
QueryInputs::Untracked => {
return false;
}

QueryInputs::NoInputs => {}
None if self.revisions.untracked => return false,
None => {}

// Check whether any of our inputs changed since the
// **last point where we were verified** (not since we
Expand All @@ -761,7 +758,7 @@ where
// R1. But our *verification* date will be R2, and we
// are only interested in finding out whether the
// input changed *again*.
QueryInputs::Tracked { inputs } => {
Some(inputs) => {
let changed_input =
inputs.slice.iter().find(|&&input| db.maybe_changed_after(input, verified_at));
if let Some(input) = changed_input {
Expand Down Expand Up @@ -793,7 +790,7 @@ where
}

fn has_untracked_input(&self) -> bool {
matches!(self.revisions.inputs, QueryInputs::Untracked)
self.revisions.untracked
}
}

Expand Down
26 changes: 15 additions & 11 deletions crates/salsa/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use dependency_graph::DependencyGraph;
pub(crate) mod local_state;
use local_state::LocalState;

use self::local_state::{ActiveQueryGuard, QueryInputs, QueryRevisions};
use self::local_state::{ActiveQueryGuard, QueryRevisions};

/// The salsa runtime stores the storage for all queries as well as
/// tracking the query stack and dependencies between cycles.
Expand Down Expand Up @@ -558,21 +558,25 @@ impl ActiveQuery {
}

pub(crate) fn revisions(&self) -> QueryRevisions {
let inputs = match &self.dependencies {
None => QueryInputs::Untracked,
let (inputs, untracked) = match &self.dependencies {
None => (None, true),

Some(dependencies) => {
Some(dependencies) => (
if dependencies.is_empty() {
QueryInputs::NoInputs
None
} else {
QueryInputs::Tracked {
inputs: ThinArc::from_header_and_iter((), dependencies.iter().copied()),
}
}
}
Some(ThinArc::from_header_and_iter((), dependencies.iter().copied()))
},
false,
),
};

QueryRevisions { changed_at: self.changed_at, inputs, durability: self.durability }
QueryRevisions {
changed_at: self.changed_at,
inputs,
untracked,
durability: self.durability,
}
}

/// Adds any dependencies from `other` into `self`.
Expand Down
20 changes: 6 additions & 14 deletions crates/salsa/src/runtime/local_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,13 @@ pub(crate) struct QueryRevisions {
/// Minimum durability of the inputs to this query.
pub(crate) durability: Durability,

/// The inputs that went into our query, if we are tracking them.
pub(crate) inputs: QueryInputs,
}

/// Every input.
#[derive(Debug, Clone)]
pub(crate) enum QueryInputs {
/// Non-empty set of inputs, fully known
Tracked { inputs: ThinArc<(), DatabaseKeyIndex> },

/// Empty set of inputs, fully known.
NoInputs,
/// Whether the input is untracked.
/// Invariant: if `untracked`, `inputs` is `None`.
/// Why is this encoded like this and not a proper enum? Struct size, this saves us 8 bytes.
pub(crate) untracked: bool,

/// Unknown quantity of inputs
Untracked,
/// The inputs that went into our query, if we are tracking them.
pub(crate) inputs: Option<ThinArc<(), DatabaseKeyIndex>>,
}

impl Default for LocalState {
Expand Down