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

Shrink QueryRevisions size by 3 words #636

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
5 changes: 0 additions & 5 deletions src/accumulator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use std::{

use accumulated::Accumulated;
use accumulated::AnyAccumulated;
use accumulated_map::AccumulatedMap;

use crate::{
cycle::CycleRecoveryStrategy,
Expand Down Expand Up @@ -149,10 +148,6 @@ impl<A: Accumulator> Ingredient for IngredientImpl<A> {
fn debug_name(&self) -> &'static str {
A::DEBUG_NAME
}

fn accumulated(&self, _db: &dyn Database, _key_index: Id) -> Option<&AccumulatedMap> {
None
}
}

impl<A> std::fmt::Debug for IngredientImpl<A>
Expand Down
44 changes: 15 additions & 29 deletions src/accumulator/accumulated_map.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::ops;

use rustc_hash::FxHashMap;

use crate::IngredientIndex;
Expand All @@ -7,10 +9,6 @@ use super::{accumulated::Accumulated, Accumulator, AnyAccumulated};
#[derive(Default, Debug)]
pub struct AccumulatedMap {
map: FxHashMap<IngredientIndex, Box<dyn AnyAccumulated>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess a more local change would be to change map here to Option<Box<FxHashMap>>. It would have the advantage that all accumualted fields remain closer together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally not a bad idea but I think (will have to verify but I am fairly certain) that moving InputAccumulatedValues back into AccumulatedMap will increase the size of QueryRevisions as the field can't be batched together with other fields that are smaller than the alignment.


/// [`InputAccumulatedValues::Empty`] if any input read during the query's execution
/// has any direct or indirect accumulated values.
inputs: InputAccumulatedValues,
}

impl AccumulatedMap {
Expand All @@ -21,21 +19,6 @@ impl AccumulatedMap {
.accumulate(value);
}

/// Adds the accumulated state of an input to this accumulated map.
pub(crate) fn add_input(&mut self, input: InputAccumulatedValues) {
if input.is_any() {
self.inputs = InputAccumulatedValues::Any;
}
}

/// Returns whether an input of the associated query has any accumulated values.
///
/// Note: Use [`InputAccumulatedValues::from_map`] to check if the associated query itself
/// or any of its inputs has accumulated values.
pub(crate) fn inputs(&self) -> InputAccumulatedValues {
self.inputs
}

pub fn extend_with_accumulated<A: Accumulator>(
&self,
index: IngredientIndex,
Expand All @@ -50,6 +33,10 @@ impl AccumulatedMap {
.unwrap()
.extend_with_accumulated(output);
}

pub fn is_empty(&self) -> bool {
self.map.is_empty()
}
}

impl Clone for AccumulatedMap {
Expand All @@ -60,7 +47,6 @@ impl Clone for AccumulatedMap {
.iter()
.map(|(&key, value)| (key, value.cloned()))
.collect(),
inputs: self.inputs,
}
}
}
Expand All @@ -70,7 +56,7 @@ impl Clone for AccumulatedMap {
/// Knowning whether any input has accumulated values makes aggregating the accumulated values
/// cheaper because we can skip over entire subtrees.
#[derive(Copy, Clone, Debug, Default)]
pub(crate) enum InputAccumulatedValues {
pub enum InputAccumulatedValues {
/// The query nor any of its inputs have any accumulated values.
#[default]
Empty,
Expand All @@ -80,14 +66,6 @@ pub(crate) enum InputAccumulatedValues {
}

impl InputAccumulatedValues {
pub(crate) fn from_map(accumulated: &AccumulatedMap) -> Self {
if accumulated.map.is_empty() {
accumulated.inputs
} else {
Self::Any
}
}

pub(crate) const fn is_any(self) -> bool {
matches!(self, Self::Any)
}
Expand All @@ -96,3 +74,11 @@ impl InputAccumulatedValues {
matches!(self, Self::Empty)
}
}

impl ops::BitOrAssign for InputAccumulatedValues {
fn bitor_assign(&mut self, rhs: Self) {
if rhs.is_any() {
*self = Self::Any;
}
}
}
21 changes: 18 additions & 3 deletions src/active_query.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::ops::Not;

use rustc_hash::FxHashMap;

use super::zalsa_local::{EdgeKind, QueryEdges, QueryOrigin, QueryRevisions};
Expand Down Expand Up @@ -53,6 +55,10 @@ pub(crate) struct ActiveQuery {
/// Stores the values accumulated to the given ingredient.
/// The type of accumulated value is erased but known to the ingredient.
pub(crate) accumulated: AccumulatedMap,

/// [`InputAccumulatedValues::Empty`] if any input read during the query's execution
/// has any accumulated values.
pub(super) accumulated_inputs: InputAccumulatedValues,
}

impl ActiveQuery {
Expand All @@ -67,6 +73,7 @@ impl ActiveQuery {
disambiguator_map: Default::default(),
tracked_struct_ids: Default::default(),
accumulated: Default::default(),
accumulated_inputs: Default::default(),
}
}

Expand All @@ -80,7 +87,7 @@ impl ActiveQuery {
self.input_outputs.insert((EdgeKind::Input, input));
self.durability = self.durability.min(durability);
self.changed_at = self.changed_at.max(revision);
self.accumulated.add_input(accumulated);
self.accumulated_inputs |= accumulated;
}

pub(super) fn add_untracked_read(&mut self, changed_at: Revision) {
Expand Down Expand Up @@ -119,13 +126,21 @@ impl ActiveQuery {
} else {
QueryOrigin::Derived(edges)
};

let accumulated = self
.accumulated
.is_empty()
.not()
.then(|| Box::new(self.accumulated));
QueryRevisions {
changed_at: self.changed_at,
origin,
durability: self.durability,
tracked_struct_ids: self.tracked_struct_ids,
accumulated: self.accumulated,
accumulated_inputs: match &accumulated {
Some(_) => InputAccumulatedValues::Any,
None => self.accumulated_inputs,
},
accumulated,
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/function.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{any::Any, fmt, sync::Arc};

use crate::{
accumulator::accumulated_map::AccumulatedMap,
accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues},
cycle::CycleRecoveryStrategy,
ingredient::fmt_index,
key::DatabaseKeyIndex,
Expand Down Expand Up @@ -249,7 +249,7 @@ where
&'db self,
db: &'db dyn Database,
key_index: Id,
) -> Option<&'db AccumulatedMap> {
) -> (Option<&'db AccumulatedMap>, InputAccumulatedValues) {
let db = db.as_view::<C::DbView>();
self.accumulated_map(db, key_index)
}
Expand Down
20 changes: 12 additions & 8 deletions src/function/accumulated.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use super::{Configuration, IngredientImpl};
use crate::accumulator::accumulated_map::InputAccumulatedValues;
use crate::zalsa_local::QueryOrigin;
use crate::{
accumulator::{self, accumulated_map::AccumulatedMap},
Expand Down Expand Up @@ -55,13 +56,13 @@ where

let ingredient = zalsa.lookup_ingredient(k.ingredient_index);
// Extend `output` with any values accumulated by `k`.
if let Some(accumulated_map) = ingredient.accumulated(db, k.key_index) {
let (accumulated_map, input) = ingredient.accumulated(db, k.key_index);
if let Some(accumulated_map) = accumulated_map {
accumulated_map.extend_with_accumulated(accumulator.index(), &mut output);

// Skip over the inputs because we know that the entire sub-graph has no accumulated values
if accumulated_map.inputs().is_empty() {
continue;
}
}
// Skip over the inputs because we know that the entire sub-graph has no accumulated values
if input.is_empty() {
continue;
}

// Find the inputs of `k` and push them onto the stack.
Expand Down Expand Up @@ -95,9 +96,12 @@ where
&'db self,
db: &'db C::DbView,
key: Id,
) -> Option<&'db AccumulatedMap> {
) -> (Option<&'db AccumulatedMap>, InputAccumulatedValues) {
// NEXT STEP: stash and refactor `fetch` to return an `&Memo` so we can make this work
let memo = self.refresh_memo(db, key);
Some(&memo.revisions.accumulated)
(
memo.revisions.accumulated.as_deref(),
memo.revisions.accumulated_inputs,
)
}
}
3 changes: 1 addition & 2 deletions src/function/fetch.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use super::{memo::Memo, Configuration, IngredientImpl};
use crate::accumulator::accumulated_map::InputAccumulatedValues;
use crate::{runtime::StampedValue, zalsa::ZalsaDatabase, AsDynDatabase as _, Id};

impl<C> IngredientImpl<C>
Expand All @@ -25,7 +24,7 @@ where
self.database_key_index(id).into(),
durability,
changed_at,
InputAccumulatedValues::from_map(&memo.revisions.accumulated),
memo.revisions.accumulated_inputs,
);

value
Expand Down
19 changes: 13 additions & 6 deletions src/function/memo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,23 +76,25 @@ impl<C: Configuration> IngredientImpl<C> {
}
QueryOrigin::Derived(_) => {
// QueryRevisions: !Clone to discourage cloning, we need it here though
let QueryRevisions {
let &QueryRevisions {
changed_at,
durability,
origin,
tracked_struct_ids,
accumulated,
ref origin,
ref tracked_struct_ids,
ref accumulated,
accumulated_inputs,
} = &memo.revisions;
// Re-assemble the memo but with the value set to `None`
Arc::new(Memo::new(
None,
memo.verified_at.load(),
QueryRevisions {
changed_at: *changed_at,
durability: *durability,
changed_at,
durability,
origin: origin.clone(),
tracked_struct_ids: tracked_struct_ids.clone(),
accumulated: accumulated.clone(),
accumulated_inputs,
},
))
}
Expand All @@ -115,6 +117,11 @@ pub(super) struct Memo<V> {
pub(super) revisions: QueryRevisions,
}

// Memo's are stored a lot, make sure their size is doesn't randomly increase.
// #[cfg(test)]
const _: [(); std::mem::size_of::<Memo<std::num::NonZeroUsize>>()] =
[(); std::mem::size_of::<[usize; 12]>()];

impl<V> Memo<V> {
pub(super) fn new(value: Option<V>, revision_now: Revision, revisions: QueryRevisions) -> Self {
Memo {
Expand Down
1 change: 1 addition & 0 deletions src/function/specify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ where
origin: QueryOrigin::Assigned(active_query_key),
tracked_struct_ids: Default::default(),
accumulated: Default::default(),
accumulated_inputs: Default::default(),
};

if let Some(old_memo) = self.get_memo_from_table_for(zalsa, key) {
Expand Down
7 changes: 5 additions & 2 deletions src/ingredient.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::{
};

use crate::{
accumulator::accumulated_map::AccumulatedMap,
accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues},
cycle::CycleRecoveryStrategy,
zalsa::{IngredientIndex, MemoIngredientIndex},
zalsa_local::QueryOrigin,
Expand Down Expand Up @@ -74,7 +74,10 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync {
&'db self,
db: &'db dyn Database,
key_index: Id,
) -> Option<&'db AccumulatedMap>;
) -> (Option<&'db AccumulatedMap>, InputAccumulatedValues) {
_ = (db, key_index);
(None, InputAccumulatedValues::Any)
}
Comment on lines -77 to +80
Copy link
Member Author

Choose a reason for hiding this comment

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

One thing to note is that I have this trait method a default impl to reduce some code noise. I imagine the intent was to force the implementor to think about this function but it does make changing signatures really annoying as well.


/// Invoked when the value `output_key` should be marked as valid in the current revision.
/// This occurs because the value for `executor`, which generated it, was marked as valid
Expand Down
8 changes: 0 additions & 8 deletions src/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,14 +275,6 @@ impl<C: Configuration> Ingredient for IngredientImpl<C> {
fn debug_name(&self) -> &'static str {
C::DEBUG_NAME
}

fn accumulated<'db>(
&'db self,
_db: &'db dyn Database,
_key_index: Id,
) -> Option<&'db crate::accumulator::accumulated_map::AccumulatedMap> {
None
}
}

impl<C: Configuration> std::fmt::Debug for IngredientImpl<C> {
Expand Down
8 changes: 0 additions & 8 deletions src/input/input_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,14 +96,6 @@ where
fn debug_name(&self) -> &'static str {
C::FIELD_DEBUG_NAMES[self.field_index]
}

fn accumulated<'db>(
&'db self,
_db: &'db dyn Database,
_key_index: Id,
) -> Option<&'db crate::accumulator::accumulated_map::AccumulatedMap> {
None
}
}

impl<C> std::fmt::Debug for FieldIngredientImpl<C>
Expand Down
8 changes: 0 additions & 8 deletions src/interned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,14 +279,6 @@ where
fn debug_name(&self) -> &'static str {
C::DEBUG_NAME
}

fn accumulated<'db>(
&'db self,
_db: &'db dyn Database,
_key_index: Id,
) -> Option<&'db crate::accumulator::accumulated_map::AccumulatedMap> {
None
}
}

impl<C> std::fmt::Debug for IngredientImpl<C>
Expand Down
8 changes: 0 additions & 8 deletions src/tracked_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -636,14 +636,6 @@ where
}

fn reset_for_new_revision(&mut self) {}

fn accumulated<'db>(
&'db self,
_db: &'db dyn Database,
_key_index: Id,
) -> Option<&'db crate::accumulator::accumulated_map::AccumulatedMap> {
None
}
}

impl<C> std::fmt::Debug for IngredientImpl<C>
Expand Down
8 changes: 0 additions & 8 deletions src/tracked_struct/tracked_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,6 @@ where
fn debug_name(&self) -> &'static str {
C::FIELD_DEBUG_NAMES[self.field_index]
}

fn accumulated<'db>(
&'db self,
_db: &'db dyn Database,
_key_index: Id,
) -> Option<&'db crate::accumulator::accumulated_map::AccumulatedMap> {
None
}
}

impl<C> std::fmt::Debug for FieldIngredientImpl<C>
Expand Down
Loading
Loading