From 0059b7c004caf896dfb8b0ea106a5793d4bde4c2 Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Fri, 22 Dec 2023 11:13:20 -0800 Subject: [PATCH] New block evaluation `Budget` system replacing `depth`. The old idea of just capping recursion depth was too weak, because it's easy to make a very expensive block without having very much recursion. So, instead, we have counters that are only decremented (while still also having a recursion guard). By putting it inside the `EvalFilter`, we avoid having multiple recursively passed parameters, and allow the original caller to both choose the budget and find out how much of it was used. Future work: Expose evaluation costs in `EvaluatedBlock` for diagnostic purposes; specifically so users can avoid running into them unexpectedly, and so we can write tests of this new logic. --- all-is-cubes/src/block.rs | 154 ++++++++++++++++--- all-is-cubes/src/block/block_def.rs | 3 + all-is-cubes/src/block/evaluated.rs | 8 + all-is-cubes/src/block/modifier.rs | 18 ++- all-is-cubes/src/block/modifier/composite.rs | 8 +- all-is-cubes/src/block/modifier/move.rs | 8 +- all-is-cubes/src/block/modifier/zoom.rs | 28 +++- all-is-cubes/src/block/tests.rs | 1 + all-is-cubes/src/block/text.rs | 20 ++- all-is-cubes/src/space/palette.rs | 1 + 10 files changed, 196 insertions(+), 53 deletions(-) diff --git a/all-is-cubes/src/block.rs b/all-is-cubes/src/block.rs index acb13f1ef..068adc354 100644 --- a/all-is-cubes/src/block.rs +++ b/all-is-cubes/src/block.rs @@ -9,6 +9,7 @@ use alloc::boxed::Box; use alloc::collections::VecDeque; use alloc::sync::Arc; use alloc::vec::Vec; +use core::cell::Cell; use core::fmt; use crate::listen::{self, Listen, Listener}; @@ -421,6 +422,7 @@ impl Block { self.evaluate2(&EvalFilter { skip_eval: false, listener: None, + budget: Default::default(), }) } @@ -444,6 +446,7 @@ impl Block { self.evaluate2(&EvalFilter { skip_eval: false, listener: Some(listener.erased()), + budget: Default::default(), }) } @@ -452,11 +455,13 @@ impl Block { /// TODO: Placeholder name. At some point we may expose `EvalFilter` directly and make /// this be just `evaluate()`. pub(crate) fn evaluate2(&self, filter: &EvalFilter) -> Result { - Ok(EvaluatedBlock::from(self.evaluate_impl(0, filter)?)) + Ok(EvaluatedBlock::from(self.evaluate_impl(filter)?)) } #[inline] - fn evaluate_impl(&self, depth: u8, filter: &EvalFilter) -> Result { + fn evaluate_impl(&self, filter: &EvalFilter) -> Result { + Budget::decrement_components(&filter.budget)?; + let mut value: MinEval = match *self.primitive() { Primitive::Indirect(ref def_ref) => def_ref.read()?.evaluate_impl(filter)?, @@ -520,17 +525,24 @@ impl Block { .intersection(block_space.bounds()) .filter(|_| !filter.skip_eval) { - Some(occupied_bounds) => block_space - .extract( - occupied_bounds, - #[inline(always)] - |extract| { - let ev = extract.block_data().evaluated(); - voxels_animation_hint |= ev.attributes.animation_hint; - Evoxel::from_block(ev) - }, - ) - .translate(-offset.to_vector()), + Some(occupied_bounds) => { + Budget::decrement_voxels( + &filter.budget, + occupied_bounds.volume().unwrap(), + )?; + + block_space + .extract( + occupied_bounds, + #[inline(always)] + |extract| { + let ev = extract.block_data().evaluated(); + voxels_animation_hint |= ev.attributes.animation_hint; + Evoxel::from_block(ev) + }, + ) + .translate(-offset.to_vector()) + } None => { // If there is no intersection, then return an empty voxel array, // with an arbitrary position. @@ -556,15 +568,14 @@ impl Block { } } - Primitive::Text { ref text, offset } => text.evaluate(offset, depth, filter)?, + Primitive::Text { ref text, offset } => text.evaluate(offset, filter)?, }; #[cfg(debug_assertions)] value.consistency_check(); for (index, modifier) in self.modifiers().iter().enumerate() { - // TODO: Extend recursion depth model to catch stacking up lots of modifiers - value = modifier.evaluate(self, index, value, depth, filter)?; + value = modifier.evaluate(self, index, value, filter)?; #[cfg(debug_assertions)] value.consistency_check(); @@ -607,6 +618,13 @@ pub(crate) struct EvalFilter { /// such as the [`Space`] referred to by a [`Primitive::Recur`] or the [`BlockDef`] /// referred to by a [`Primitive::Indirect`]. pub listener: Option>, + + /// How much computation may be spent on performing the evaluation. + /// + /// If the budget is exhausted, evaluation returns [`EvalBlockError::StackOverflow`]. + /// + /// Outside of special circumstances, use [`Budget::default()`] here. + pub budget: Cell, } impl Default for EvalFilter { @@ -616,19 +634,11 @@ impl Default for EvalFilter { Self { skip_eval: Default::default(), listener: Default::default(), + budget: Default::default(), } } } -/// Recursion limiter helper for evaluate. -fn next_depth(depth: u8) -> Result { - if depth > 32 { - Err(EvalBlockError::StackOverflow) - } else { - Ok(depth + 1) - } -} - // Manual implementations of Eq and Hash ensure that the [`BlockPtr`] storage // choices do not affect equality. impl PartialEq for Block { @@ -941,6 +951,100 @@ mod conversions_for_indirect { } } +/// Computation budget for block evaluations. +/// +/// This is used inside an [`EvalFilter`]. +/// +/// In principle, what we want is a time budget, but in order to offer determinism and +/// comprehensibility, it is instead made up of multiple discrete quantities. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) struct Budget { + /// Number of [`Primitive`]s and [`Modifier`]s. + components: u32, + + /// Number of individual voxels produced (e.g. by a [`Primitive::Recur`]) or altered + /// (e.g. by a [`Modifier::Composite`]). + voxels: u32, + + /// Number of levels of evaluation recursion permitted. + /// + /// Recursion occurs when a primitive or modifier which itself contains a [`Block`] is + /// evaluated; currently, these are [`Primitive::Text`] and [`Modifier::Composite`]. + /// + /// This must be set low enough to avoid Rust stack overflows which cannot be recovered from. + /// Unlike the other budget parameters, this is not cumulative over the entire evaluation. + recursion: u8, + + /// Number of recursion levels actually used by the evaluation. + /// This is tracked separately so that it can be reported afterward, + /// whereas the other counters are only ever decremented and so a subtraction suffices. + recursion_used: u8, +} + +impl Budget { + pub(crate) fn decrement_components(cell: &Cell) -> Result<(), EvalBlockError> { + let mut budget = cell.get(); + match budget.components.checked_sub(1) { + Some(updated) => budget.components = updated, + None => return Err(EvalBlockError::StackOverflow), + } + cell.set(budget); + Ok(()) + } + + pub(crate) fn decrement_voxels( + cell: &Cell, + amount: usize, + ) -> Result<(), EvalBlockError> { + let mut budget = cell.get(); + match u32::try_from(amount) + .ok() + .and_then(|amount| budget.voxels.checked_sub(amount)) + { + Some(updated) => budget.voxels = updated, + None => return Err(EvalBlockError::StackOverflow), + } + cell.set(budget); + Ok(()) + } + + pub(crate) fn recurse(cell: &Cell) -> Result, EvalBlockError> { + let current = cell.get(); + let mut recursed = current; + match recursed.recursion.checked_sub(1) { + Some(updated) => recursed.recursion = updated, + None => return Err(EvalBlockError::StackOverflow), + } + cell.set(recursed); + Ok(BudgetRecurseGuard { cell }) + } +} + +impl Default for Budget { + /// Returns the standard budget for starting any evaluation. + fn default() -> Self { + Self { + components: 1000, + voxels: 64 * 64 * 128, + recursion: 30, + recursion_used: 0, + } + } +} + +#[must_use] +pub(crate) struct BudgetRecurseGuard<'a> { + cell: &'a Cell, +} + +impl Drop for BudgetRecurseGuard<'_> { + fn drop(&mut self) { + let mut budget = self.cell.get(); + budget.recursion = budget.recursion.checked_add(1).unwrap(); + self.cell.set(budget); + } +} + /// Notification when an [`EvaluatedBlock`] result changes. #[derive(Clone, Debug, Eq, Hash, PartialEq)] #[non_exhaustive] diff --git a/all-is-cubes/src/block/block_def.rs b/all-is-cubes/src/block/block_def.rs index 8aa2529d5..3a1a21e45 100644 --- a/all-is-cubes/src/block/block_def.rs +++ b/all-is-cubes/src/block/block_def.rs @@ -99,6 +99,7 @@ impl BlockDef { let &block::EvalFilter { skip_eval, ref listener, + budget: _, // already accounted in the caller } = filter; if let Some(listener) = listener { @@ -132,6 +133,7 @@ impl BlockDefState { .evaluate2(&block::EvalFilter { skip_eval: false, listener: Some(block_listener.erased()), + budget: Default::default(), }) .map(MinEval::from); @@ -165,6 +167,7 @@ impl BlockDefState { .evaluate2(&block::EvalFilter { skip_eval: false, listener: None, // we already have a listener installed + budget: Default::default(), }) .map(MinEval::from); diff --git a/all-is-cubes/src/block/evaluated.rs b/all-is-cubes/src/block/evaluated.rs index 2f21a6d5d..5bf461809 100644 --- a/all-is-cubes/src/block/evaluated.rs +++ b/all-is-cubes/src/block/evaluated.rs @@ -602,6 +602,14 @@ impl Evoxels { } } + /// Returns the count of voxels, aka [`Vol::volume()`] at the resolution. + pub fn count(&self) -> usize { + match self { + Evoxels::One(_) => 1, + Evoxels::Many(_, voxels) => voxels.volume(), + } + } + /// If this has a resolution of 1, then return that single voxel. #[inline] pub fn single_voxel(&self) -> Option { diff --git a/all-is-cubes/src/block/modifier.rs b/all-is-cubes/src/block/modifier.rs index fadcb34b1..6efc4d3bf 100644 --- a/all-is-cubes/src/block/modifier.rs +++ b/all-is-cubes/src/block/modifier.rs @@ -79,20 +79,23 @@ impl Modifier { /// * `this_modifier_index` is the index in `block.modifiers()` of `self`. /// * `value` is the output of the preceding modifier or primitive, which is what the /// current modifier should be applied to. - /// * `depth` is the current block evaluation recursion depth (which is *not* - /// incremented by modifiers; TODO: define a computation limit strategy). + /// * `filter` controls evaluation options and listening, and its budget is decremented by + /// 1 component (the modifier itself) plus as many voxels and additional components as the + /// particular modifier needs to calculate. pub(crate) fn evaluate( &self, block: &Block, this_modifier_index: usize, mut value: MinEval, - depth: u8, filter: &block::EvalFilter, ) -> Result { + block::Budget::decrement_components(&filter.budget)?; + Ok(match *self { Modifier::Quote(Quote { suppress_ambient }) => { value.attributes.tick_action = None; if suppress_ambient { + block::Budget::decrement_voxels(&filter.budget, value.voxels.count())?; for voxel in value.voxels.as_vol_mut().as_linear_mut().iter_mut() { voxel.emission = Rgb::ZERO; } @@ -105,6 +108,7 @@ impl Modifier { // Skip computation of transforms value } else { + block::Budget::decrement_voxels(&filter.budget, value.voxels.count())?; // TODO: Add a shuffle-in-place rotation operation to Vol and try implementing this using that, which should have less arithmetic involved than these matrix ops let resolution = value.resolution(); let inner_to_outer = rotation.to_positive_octant_transform(resolution.into()); @@ -130,13 +134,11 @@ impl Modifier { } } - Modifier::Composite(ref c) => c.evaluate(value, depth, filter)?, + Modifier::Composite(ref c) => c.evaluate(value, filter)?, - Modifier::Zoom(ref z) => z.evaluate(value)?, + Modifier::Zoom(ref z) => z.evaluate(value, filter)?, - Modifier::Move(ref m) => { - m.evaluate(block, this_modifier_index, value, depth, filter)? - } + Modifier::Move(ref m) => m.evaluate(block, this_modifier_index, value, filter)?, }) } diff --git a/all-is-cubes/src/block/modifier/composite.rs b/all-is-cubes/src/block/modifier/composite.rs index 74b17b0f2..54fc662d0 100644 --- a/all-is-cubes/src/block/modifier/composite.rs +++ b/all-is-cubes/src/block/modifier/composite.rs @@ -118,7 +118,6 @@ impl Composite { pub(super) fn evaluate( &self, mut dst_evaluated: MinEval, - depth: u8, filter: &block::EvalFilter, ) -> Result { let Composite { @@ -130,7 +129,10 @@ impl Composite { // The destination block is already evaluated (it is the input to this // modifier), but we need to evaluate the source block. - let mut src_evaluated = source.evaluate_impl(block::next_depth(depth)?, filter)?; + let mut src_evaluated = { + let _recursion_scope = block::Budget::recurse(&filter.budget)?; + source.evaluate_impl(filter)? + }; if filter.skip_eval { return Ok(dst_evaluated); @@ -163,6 +165,8 @@ impl Composite { let output_bounds = operator.bounds(src_bounds_scaled, dst_bounds_scaled); + block::Budget::decrement_voxels(&filter.budget, output_bounds.volume().unwrap())?; + let attributes = block::BlockAttributes { display_name: dst_att.display_name, // TODO merge selectable: src_att.selectable | dst_att.selectable, diff --git a/all-is-cubes/src/block/modifier/move.rs b/all-is-cubes/src/block/modifier/move.rs index 5ee1e44f7..120b91181 100644 --- a/all-is-cubes/src/block/modifier/move.rs +++ b/all-is-cubes/src/block/modifier/move.rs @@ -68,7 +68,6 @@ impl Move { block: &Block, this_modifier_index: usize, mut input: MinEval, - depth: u8, filter: &block::EvalFilter, ) -> Result { let Move { @@ -81,11 +80,11 @@ impl Move { // don't interfere with movement or cause duplication. // (In the future we may want a more nuanced policy that allows internal changes, // but that will probably involve refining tick_action processing.) + // TODO: This can be done more cleanly by having an evaluate function on Quote itself. input = Modifier::from(block::Quote::default()).evaluate( block, this_modifier_index, input, - depth, filter, )?; @@ -148,6 +147,11 @@ impl Move { Ok(match displaced_bounds { Some(displaced_bounds) => { + block::Budget::decrement_voxels( + &filter.budget, + displaced_bounds.volume().unwrap(), + )?; + let displaced_voxels = match &input.voxels { Evoxels::Many(_, voxels) => Evoxels::Many( effective_resolution, diff --git a/all-is-cubes/src/block/modifier/zoom.rs b/all-is-cubes/src/block/modifier/zoom.rs index bcac20c60..1ced3b16b 100644 --- a/all-is-cubes/src/block/modifier/zoom.rs +++ b/all-is-cubes/src/block/modifier/zoom.rs @@ -59,12 +59,18 @@ impl Zoom { } } - pub(super) fn evaluate(&self, input: MinEval) -> Result { + pub(super) fn evaluate( + &self, + input: MinEval, + filter: &block::EvalFilter, + ) -> Result { let Zoom { offset: offset_in_zoomed_blocks, scale, } = *self; + // TODO: respect filter.skip_eval + // TODO: To efficiently implement this, we should be able to run in a phase // *before* the `Primitive` evaluation, which allows us to reduce how many // of the primitive voxels are evaluated. (Modifier::Move will also benefit.) @@ -97,13 +103,19 @@ impl Zoom { attributes, voxels: Evoxels::One(Evoxel::AIR), }, - Some(intersected_bounds) => MinEval { - attributes, - voxels: Evoxels::Many( - zoom_resolution, - Vol::from_fn(intersected_bounds, |p| voxels[p + voxel_offset]), - ), - }, + Some(intersected_bounds) => { + block::Budget::decrement_voxels( + &filter.budget, + intersected_bounds.volume().unwrap(), + )?; + MinEval { + attributes, + voxels: Evoxels::Many( + zoom_resolution, + Vol::from_fn(intersected_bounds, |p| voxels[p + voxel_offset]), + ), + } + } } } }) diff --git a/all-is-cubes/src/block/tests.rs b/all-is-cubes/src/block/tests.rs index c5093b5c9..41d9fd1ed 100644 --- a/all-is-cubes/src/block/tests.rs +++ b/all-is-cubes/src/block/tests.rs @@ -37,6 +37,7 @@ fn listen( .evaluate2(&block::EvalFilter { skip_eval: true, listener: Some(listener.erased()), + budget: Default::default(), }) .map(|_| ()) } diff --git a/all-is-cubes/src/block/text.rs b/all-is-cubes/src/block/text.rs index f7e2ae4d5..f6b03b9d4 100644 --- a/all-is-cubes/src/block/text.rs +++ b/all-is-cubes/src/block/text.rs @@ -207,7 +207,6 @@ impl Text { pub(crate) fn evaluate( &self, block_offset: GridVector, - _depth: u8, filter: &super::EvalFilter, ) -> Result { if filter.skip_eval { @@ -216,13 +215,18 @@ impl Text { return Ok(block::AIR_EVALUATED_MIN); // placeholder value } - let ev_foreground = Evoxel::from_block(&self.foreground.evaluate2(filter)?); - let brush = match self.outline { - Some(ref block) => Brush::Outline { - foreground: ev_foreground, - outline: Evoxel::from_block(&block.evaluate2(filter)?), - }, - None => Brush::Plain(ev_foreground), + // Evaluate blocks making up the brush + let brush = { + let _recursion_scope = block::Budget::recurse(&filter.budget)?; + + let ev_foreground = Evoxel::from_block(&self.foreground.evaluate2(filter)?); + match self.outline { + Some(ref block) => Brush::Outline { + foreground: ev_foreground, + outline: Evoxel::from_block(&block.evaluate2(filter)?), + }, + None => Brush::Plain(ev_foreground), + } }; self.with_transform_and_drawable( diff --git a/all-is-cubes/src/space/palette.rs b/all-is-cubes/src/space/palette.rs index a0f8879f4..3e7ffffc8 100644 --- a/all-is-cubes/src/space/palette.rs +++ b/all-is-cubes/src/space/palette.rs @@ -461,6 +461,7 @@ impl SpaceBlockData { let evaluated = match block.evaluate2(&block::EvalFilter { skip_eval: false, listener: Some(block_listener.clone()), + budget: Default::default(), }) { Ok(ev) => ev, Err(err) => {