From 6b6cdae7ce95681d6a1194be70b375494166a8c6 Mon Sep 17 00:00:00 2001 From: Martin Haug Date: Sun, 27 Jun 2021 12:28:40 +0200 Subject: [PATCH 1/5] Testing for incremental Also, constraint bugfixes. --- src/geom/length.rs | 5 ++ src/layout/grid.rs | 10 ++- src/layout/incremental.rs | 155 +++++++++++++++++++++++++++++++++++--- src/layout/mod.rs | 14 +--- src/layout/pad.rs | 7 +- src/layout/par.rs | 21 +++++- src/layout/stack.rs | 13 +++- tests/typeset.rs | 56 +++++++++++++- 8 files changed, 245 insertions(+), 36 deletions(-) diff --git a/src/geom/length.rs b/src/geom/length.rs index ecfe56163..08bf3111b 100644 --- a/src/geom/length.rs +++ b/src/geom/length.rs @@ -108,6 +108,11 @@ impl Length { self.raw + 1e-6 >= other.raw } + /// Compares to lengths for whether they are approximately equal. + pub fn approx_eq(self, other: Self) -> bool { + self == other || (self - other).to_raw().abs() < 1e-6 + } + /// Whether the length is zero. pub fn is_zero(self) -> bool { self.raw == 0.0 diff --git a/src/layout/grid.rs b/src/layout/grid.rs index d2976502a..996ce7c26 100644 --- a/src/layout/grid.rs +++ b/src/layout/grid.rs @@ -64,6 +64,8 @@ struct GridLayouter<'a> { children: &'a [AnyNode], /// The region to layout into. regions: Regions, + /// The original expand state of the target region. + original_expand: Spec, /// Resolved column sizes. rcols: Vec, /// The full main size of the current region. @@ -135,7 +137,8 @@ impl<'a> GridLayouter<'a> { let full = regions.current.get(main); let rcols = vec![Length::zero(); cols.len()]; - // We use the regions only for auto row measurement. + // We use the regions only for auto row measurement and constraints. + let original_expand = regions.expand; regions.expand = Gen::new(true, false).to_spec(main); Self { @@ -144,8 +147,9 @@ impl<'a> GridLayouter<'a> { cols, rows, children: &grid.children, - constraints: Constraints::new(regions.expand), + constraints: Constraints::new(original_expand), regions, + original_expand, rcols, lrows: vec![], full, @@ -506,7 +510,7 @@ impl<'a> GridLayouter<'a> { self.used.main = Length::zero(); self.fr = Fractional::zero(); self.finished.push(output.constrain(self.constraints)); - self.constraints = Constraints::new(self.regions.expand); + self.constraints = Constraints::new(self.original_expand); } /// Get the node in the cell in column `x` and row `y`. diff --git a/src/layout/incremental.rs b/src/layout/incremental.rs index a5c3cea35..e1d1ffbac 100644 --- a/src/layout/incremental.rs +++ b/src/layout/incremental.rs @@ -7,13 +7,15 @@ use super::*; pub struct LayoutCache { /// Maps from node hashes to the resulting frames and regions in which the /// frames are valid. - pub frames: HashMap, + pub frames: HashMap>, + /// In how many compilations this cache has been used. + age: usize, } impl LayoutCache { /// Create a new, empty layout cache. pub fn new() -> Self { - Self { frames: HashMap::new() } + Self { frames: HashMap::new(), age: 0 } } /// Clear the cache. @@ -26,12 +28,68 @@ impl LayoutCache { where F: FnMut(usize) -> bool, { - self.frames.retain(|_, entry| f(entry.level)); + for (_, hash_ident) in self.frames.iter_mut() { + hash_ident.retain(|entry| f(entry.level)); + } } /// Amount of items in the cache. pub fn len(&self) -> usize { - self.frames.len() + self.frames.iter().map(|(_, e)| e.len()).sum() + } + + /// Prepare the cache for the next round of compilation + pub fn turnaround(&mut self) { + self.age += 1; + for entry in self.frames.iter_mut().flat_map(|(_, x)| x.iter_mut()) { + for i in 0 .. (entry.temperature.len() - 1) { + entry.temperature[i] = entry.temperature[i + 1]; + } + *entry.temperature.last_mut().unwrap() = Some(0); + } + } + + /// What is the deepest level in the cache? + pub fn levels(&self) -> usize { + self.frames + .iter() + .flat_map(|(_, x)| x) + .map(|entry| entry.level + 1) + .max() + .unwrap_or(0) + } + + /// Fetches the appropriate entry from the cache if there is any. + pub fn get( + &mut self, + hash: u64, + regions: Regions, + ) -> Option>>> { + self.frames.get_mut(&hash).and_then(|frames| { + for frame in frames { + let res = frame.check(regions.clone()); + if res.is_some() { + return res; + } + } + + None + }) + } + + /// Inserts a new frame set into the cache. + pub fn insert( + &mut self, + hash: u64, + frames: Vec>>, + level: usize, + ) { + let entry = FramesEntry::new(frames, level); + if let Some(variants) = self.frames.get_mut(&hash) { + variants.push(entry); + } else { + self.frames.insert(hash, vec![entry]); + } } } @@ -42,19 +100,72 @@ pub struct FramesEntry { pub frames: Vec>>, /// How nested the frame was in the context is was originally appearing in. pub level: usize, + /// How much the element was accessed during the last five compilations, the + /// most current one being the last element. `None` variants indicate that + /// the element is younger than five compilations. + temperature: [Option; 5], } impl FramesEntry { + /// Construct a new instance. + pub fn new(frames: Vec>>, level: usize) -> Self { + let mut temperature = [None; 5]; + temperature[4] = Some(0); + Self { frames, level, temperature } + } + /// Checks if the cached [`Frame`] is valid for the given regions. - pub fn check(&self, mut regions: Regions) -> Option>>> { + pub fn check(&mut self, mut regions: Regions) -> Option>>> { for (i, frame) in self.frames.iter().enumerate() { if (i != 0 && !regions.next()) || !frame.constraints.check(®ions) { return None; } } + let tmp = self.temperature.get_mut(4).unwrap(); + *tmp = Some(tmp.map_or(1, |x| x + 1)); + Some(self.frames.clone()) } + + /// Get the amount of compilation cycles this item has remained in the + /// cache. + pub fn age(&self) -> usize { + let mut age = 0; + for &temp in self.temperature.iter().rev() { + if temp.is_none() { + break; + } + age += 1; + } + age + } + + /// Get the amount of consecutive cycles in which this item has not + /// been used. + pub fn cooldown(&self) -> usize { + let mut age = 0; + for (i, &temp) in self.temperature.iter().enumerate().rev() { + match temp { + Some(temp) => { + if temp > 0 { + return self.temperature.len() - 1 - i; + } + } + None => { + return age; + } + } + age += 1 + } + + age + } + + /// Whether this element was used in the last compilation cycle. + pub fn hit(&self) -> bool { + self.temperature.last().unwrap().unwrap_or(0) != 0 + } } #[derive(Debug, Copy, Clone, PartialEq)] @@ -107,15 +218,22 @@ impl Constraints { let base = regions.base.to_spec(); let current = regions.current.to_spec(); - current.eq_by(&self.min, |x, y| y.map_or(true, |y| x >= &y)) + let res = current.eq_by(&self.min, |&x, y| y.map_or(true, |y| x.fits(y))) && current.eq_by(&self.max, |x, y| y.map_or(true, |y| x < &y)) - && current.eq_by(&self.exact, |x, y| y.map_or(true, |y| x == &y)) - && base.eq_by(&self.base, |x, y| y.map_or(true, |y| x == &y)) + && current.eq_by(&self.exact, |&x, y| y.map_or(true, |y| x.approx_eq(y))) + && base.eq_by(&self.base, |&x, y| y.map_or(true, |y| x.approx_eq(y))); + + res } /// Changes all constraints by adding the argument to them if they are set. - pub fn mutate(&mut self, size: Size) { - for x in &mut [self.min, self.max, self.exact, self.base] { + pub fn mutate(&mut self, size: Size, regions: &Regions) { + for x in std::array::IntoIter::new([ + &mut self.min, + &mut self.max, + &mut self.exact, + &mut self.base, + ]) { if let Some(horizontal) = x.horizontal.as_mut() { *horizontal += size.width; } @@ -123,6 +241,23 @@ impl Constraints { *vertical += size.height; } } + + self.exact = zip(self.exact, regions.current.to_spec(), |_, o| o); + self.base = zip(self.base, regions.base.to_spec(), |_, o| o); + } +} + +fn zip( + one: Spec>, + other: Spec, + mut f: F, +) -> Spec> +where + F: FnMut(Length, Length) -> Length, +{ + Spec { + vertical: one.vertical.map(|r| f(r, other.vertical)), + horizontal: one.horizontal.map(|r| f(r, other.horizontal)), } } diff --git a/src/layout/mod.rs b/src/layout/mod.rs index 10c30f419..dc819a164 100644 --- a/src/layout/mod.rs +++ b/src/layout/mod.rs @@ -102,18 +102,10 @@ impl Layout for AnyNode { regions: &Regions, ) -> Vec>> { ctx.level += 1; - let frames = ctx - .cache - .layout - .frames - .get(&self.hash) - .and_then(|x| x.check(regions.clone())) - .unwrap_or_else(|| { + let frames = + ctx.cache.layout.get(self.hash, regions.clone()).unwrap_or_else(|| { let frames = self.node.layout(ctx, regions); - ctx.cache.layout.frames.insert(self.hash, FramesEntry { - frames: frames.clone(), - level: ctx.level - 1, - }); + ctx.cache.layout.insert(self.hash, frames.clone(), ctx.level - 1); frames }); ctx.level -= 1; diff --git a/src/layout/pad.rs b/src/layout/pad.rs index 9d432e797..cfde07192 100644 --- a/src/layout/pad.rs +++ b/src/layout/pad.rs @@ -15,6 +15,7 @@ impl Layout for PadNode { ctx: &mut LayoutContext, regions: &Regions, ) -> Vec>> { + let original = regions.clone(); let mut regions = regions.map(|size| size - self.padding.resolve(size).size()); let mut frames = self.child.layout(ctx, ®ions); @@ -28,13 +29,13 @@ impl Layout for PadNode { let prev = std::mem::take(&mut frame.item); new.push_frame(origin, prev); - frame.constraints.mutate(padding.size() * -1.0); + frame.constraints.mutate(padding.size(), &original); if self.padding.left.is_relative() || self.padding.right.is_relative() { - frame.constraints.base.horizontal = Some(regions.base.width); + frame.constraints.base.horizontal = Some(original.base.width); } if self.padding.top.is_relative() || self.padding.bottom.is_relative() { - frame.constraints.base.vertical = Some(regions.base.height); + frame.constraints.base.vertical = Some(original.base.height); } regions.next(); diff --git a/src/layout/par.rs b/src/layout/par.rs index 2208f9ee3..0cf1cd23a 100644 --- a/src/layout/par.rs +++ b/src/layout/par.rs @@ -213,10 +213,18 @@ impl<'a> ParLayouter<'a> { while !stack.regions.current.height.fits(line.size.height) && !stack.regions.in_full_last() { - stack.constraints.max.vertical.set_min(line.size.height); + stack.constraints.max.vertical.set_min( + stack.full.height - stack.regions.current.height + line.size.height, + ); stack.finish_region(ctx); } + if !stack.regions.current.height.fits(line.size.height) + && stack.regions.in_full_last() + { + stack.overflowing = true; + } + // If the line does not fit horizontally or we have a mandatory // line break (i.e. due to "\n"), we push the line into the // stack. @@ -303,11 +311,13 @@ impl ParItem<'_> { /// Stacks lines on top of each other. struct LineStack<'a> { line_spacing: Length, + full: Size, regions: Regions, size: Size, lines: Vec>, finished: Vec>>, constraints: Constraints, + overflowing: bool, } impl<'a> LineStack<'a> { @@ -316,10 +326,12 @@ impl<'a> LineStack<'a> { Self { line_spacing, constraints: Constraints::new(regions.expand), + full: regions.current, regions, size: Size::zero(), lines: vec![], finished: vec![], + overflowing: false, } } @@ -343,6 +355,12 @@ impl<'a> LineStack<'a> { self.constraints.exact.horizontal = Some(self.regions.current.width); } + if self.overflowing { + self.constraints.min.vertical = None; + self.constraints.max.vertical = None; + self.constraints.exact = self.full.to_spec().map(Some); + } + let mut output = Frame::new(self.size, self.size.height); let mut offset = Length::zero(); let mut first = true; @@ -362,6 +380,7 @@ impl<'a> LineStack<'a> { self.finished.push(output.constrain(self.constraints)); self.regions.next(); + self.full = self.regions.current; self.constraints = Constraints::new(self.regions.expand); self.size = Size::zero(); } diff --git a/src/layout/stack.rs b/src/layout/stack.rs index 2a0b28060..92a1337fd 100644 --- a/src/layout/stack.rs +++ b/src/layout/stack.rs @@ -86,8 +86,8 @@ impl<'a> StackLayouter<'a> { Self { stack, main, + constraints: Constraints::new(expand), expand, - constraints: Constraints::new(regions.expand), regions, full, used: Gen::zero(), @@ -145,7 +145,10 @@ impl<'a> StackLayouter<'a> { while !self.regions.current.get(self.main).fits(size.main) && !self.regions.in_full_last() { - self.constraints.max.get_mut(self.main).set_min(size.main); + self.constraints + .max + .get_mut(self.main) + .set_min(size.main + self.used.main); self.finish_region(); } @@ -188,7 +191,9 @@ impl<'a> StackLayouter<'a> { // Make sure the stack's size satisfies the aspect ratio. if let Some(aspect) = self.stack.aspect { - self.constraints.exact = self.regions.current.to_spec().map(Some); + self.constraints.exact = self.full.to_spec().map(Some); + self.constraints.min = Spec::splat(None); + self.constraints.max = Spec::splat(None); let width = size .width .max(aspect.into_inner() * size.height) @@ -244,6 +249,6 @@ impl<'a> StackLayouter<'a> { self.used = Gen::zero(); self.ruler = Align::Start; self.finished.push(output.constrain(self.constraints)); - self.constraints = Constraints::new(self.regions.expand); + self.constraints = Constraints::new(expand); } } diff --git a/tests/typeset.rs b/tests/typeset.rs index c5f31e61f..9e0c95966 100644 --- a/tests/typeset.rs +++ b/tests/typeset.rs @@ -224,10 +224,8 @@ fn test_part( state.page.size = Size::new(Length::pt(120.0), Length::inf()); state.page.margins = Sides::splat(Some(Length::pt(10.0).into())); - // Clear cache between tests (for now). - cache.layout.clear(); - - let mut pass = typst::typeset(loader, cache, Some(src_path), &src, &scope, state); + let mut pass = + typst::typeset(loader, cache, Some(src_path), &src, &scope, state.clone()); if !compare_ref { pass.output.clear(); @@ -266,6 +264,56 @@ fn test_part( } } + let reference_cache = cache.layout.clone(); + for level in 0 .. reference_cache.levels() { + cache.layout = reference_cache.clone(); + cache.layout.retain(|x| x == level); + if cache.layout.frames.is_empty() { + continue; + } + + cache.layout.turnaround(); + + let mut cached_result = + typst::typeset(loader, cache, Some(src_path), &src, &scope, state.clone()); + + if !compare_ref { + cached_result.output.clear(); + } + + let misses: Vec<_> = cache + .layout + .frames + .iter() + .flat_map(|(_, e)| e) + .filter(|e| e.level == level && !e.hit() && e.age() == 2) + .collect(); + + if !misses.is_empty() { + ok = false; + let mut miss_count = 0; + for miss in misses { + dbg!(miss.frames.iter().map(|f| f.constraints).collect::>()); + miss_count += 1; + } + println!( + " Recompilation had {} cache misses on level {} (Subtest {}) ❌", + miss_count, level, i + ); + } + + if cached_result != pass { + ok = false; + println!( + " Recompilation of subtest {} differs from clean pass ❌", + i + ); + } + } + + cache.layout = reference_cache; + cache.layout.turnaround(); + (ok, compare_ref, pass.output) } From 57bd3e23c79878d106ab8be17c71caca6c4f5a7c Mon Sep 17 00:00:00 2001 From: Martin Date: Sun, 27 Jun 2021 17:08:40 +0200 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Laurenz --- src/geom/length.rs | 2 +- src/layout/incremental.rs | 2 +- src/layout/par.rs | 16 +++++++++++----- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/geom/length.rs b/src/geom/length.rs index 08bf3111b..951cef4f6 100644 --- a/src/geom/length.rs +++ b/src/geom/length.rs @@ -108,7 +108,7 @@ impl Length { self.raw + 1e-6 >= other.raw } - /// Compares to lengths for whether they are approximately equal. + /// Compares two lengths for whether they are approximately equal. pub fn approx_eq(self, other: Self) -> bool { self == other || (self - other).to_raw().abs() < 1e-6 } diff --git a/src/layout/incremental.rs b/src/layout/incremental.rs index e1d1ffbac..953b76bed 100644 --- a/src/layout/incremental.rs +++ b/src/layout/incremental.rs @@ -101,7 +101,7 @@ pub struct FramesEntry { /// How nested the frame was in the context is was originally appearing in. pub level: usize, /// How much the element was accessed during the last five compilations, the - /// most current one being the last element. `None` variants indicate that + /// most recent one being the last element. `None` variants indicate that /// the element is younger than five compilations. temperature: [Option; 5], } diff --git a/src/layout/par.rs b/src/layout/par.rs index 0cf1cd23a..40cb84f2d 100644 --- a/src/layout/par.rs +++ b/src/layout/par.rs @@ -219,12 +219,18 @@ impl<'a> ParLayouter<'a> { stack.finish_region(ctx); } - if !stack.regions.current.height.fits(line.size.height) - && stack.regions.in_full_last() - { - stack.overflowing = true; + // If the line does not fit vertically, we start a new region. + while !stack.regions.current.height.fits(line.size.height) { + if stack.regions.in_full_last() { + stack.overflowing = true; + break; + } + + stack.constraints.max.vertical.set_min( + stack.full.height - stack.regions.current.height + line.size.height, + ); + stack.finish_region(ctx); } - // If the line does not fit horizontally or we have a mandatory // line break (i.e. due to "\n"), we push the line into the // stack. From 9bd8b7ddac046f581dc750e148147901d08cb0f4 Mon Sep 17 00:00:00 2001 From: Martin Haug Date: Sun, 27 Jun 2021 18:06:39 +0200 Subject: [PATCH 3/5] Code review, new stack test --- src/layout/grid.rs | 12 ++-- src/layout/incremental.rs | 117 ++++++++++++++++++------------------- src/layout/pad.rs | 3 +- src/layout/par.rs | 6 +- src/layout/stack.rs | 18 +++++- tests/ref/layout/stack.png | Bin 264 -> 325 bytes tests/typ/layout/stack.typ | 9 +++ tests/typeset.rs | 56 +++++++++--------- 8 files changed, 124 insertions(+), 97 deletions(-) diff --git a/src/layout/grid.rs b/src/layout/grid.rs index 996ce7c26..33fce0642 100644 --- a/src/layout/grid.rs +++ b/src/layout/grid.rs @@ -56,6 +56,8 @@ struct GridLayouter<'a> { cross: SpecAxis, /// The axis of the main direction. main: SpecAxis, + /// The original expand state of the target region. + expand: Spec, /// The column tracks including gutter tracks. cols: Vec, /// The row tracks including gutter tracks. @@ -64,8 +66,6 @@ struct GridLayouter<'a> { children: &'a [AnyNode], /// The region to layout into. regions: Regions, - /// The original expand state of the target region. - original_expand: Spec, /// Resolved column sizes. rcols: Vec, /// The full main size of the current region. @@ -138,7 +138,7 @@ impl<'a> GridLayouter<'a> { let rcols = vec![Length::zero(); cols.len()]; // We use the regions only for auto row measurement and constraints. - let original_expand = regions.expand; + let expand = regions.expand; regions.expand = Gen::new(true, false).to_spec(main); Self { @@ -147,9 +147,9 @@ impl<'a> GridLayouter<'a> { cols, rows, children: &grid.children, - constraints: Constraints::new(original_expand), + constraints: Constraints::new(expand), regions, - original_expand, + expand, rcols, lrows: vec![], full, @@ -510,7 +510,7 @@ impl<'a> GridLayouter<'a> { self.used.main = Length::zero(); self.fr = Fractional::zero(); self.finished.push(output.constrain(self.constraints)); - self.constraints = Constraints::new(self.original_expand); + self.constraints = Constraints::new(self.expand); } /// Get the node in the cell in column `x` and row `y`. diff --git a/src/layout/incremental.rs b/src/layout/incremental.rs index 953b76bed..a3051d0ff 100644 --- a/src/layout/incremental.rs +++ b/src/layout/incremental.rs @@ -1,4 +1,5 @@ -use std::{collections::HashMap, ops::Deref}; +use std::collections::{hash_map::Entry, HashMap}; +use std::ops::Deref; use super::*; @@ -6,7 +7,9 @@ use super::*; #[derive(Default, Debug, Clone)] pub struct LayoutCache { /// Maps from node hashes to the resulting frames and regions in which the - /// frames are valid. + /// frames are valid. The right hand side of the hash map is a vector of + /// results because across one or more compilations, multiple different + /// layouts of the same node may have been requested. pub frames: HashMap>, /// In how many compilations this cache has been used. age: usize, @@ -28,16 +31,11 @@ impl LayoutCache { where F: FnMut(usize) -> bool, { - for (_, hash_ident) in self.frames.iter_mut() { - hash_ident.retain(|entry| f(entry.level)); + for (_, entries) in self.frames.iter_mut() { + entries.retain(|entry| f(entry.level)); } } - /// Amount of items in the cache. - pub fn len(&self) -> usize { - self.frames.iter().map(|(_, e)| e.len()).sum() - } - /// Prepare the cache for the next round of compilation pub fn turnaround(&mut self) { self.age += 1; @@ -45,11 +43,11 @@ impl LayoutCache { for i in 0 .. (entry.temperature.len() - 1) { entry.temperature[i] = entry.temperature[i + 1]; } - *entry.temperature.last_mut().unwrap() = Some(0); + *entry.temperature.last_mut().unwrap() = 0; } } - /// What is the deepest level in the cache? + /// The amount of levels stored in the cache. pub fn levels(&self) -> usize { self.frames .iter() @@ -85,16 +83,17 @@ impl LayoutCache { level: usize, ) { let entry = FramesEntry::new(frames, level); - if let Some(variants) = self.frames.get_mut(&hash) { - variants.push(entry); - } else { - self.frames.insert(hash, vec![entry]); + match self.frames.entry(hash) { + Entry::Occupied(o) => o.into_mut().push(entry), + Entry::Vacant(v) => { + v.insert(vec![entry]); + } } } } -#[derive(Debug, Clone)] /// Cached frames from past layouting. +#[derive(Debug, Clone)] pub struct FramesEntry { /// The cached frames for a node. pub frames: Vec>>, @@ -103,15 +102,20 @@ pub struct FramesEntry { /// How much the element was accessed during the last five compilations, the /// most recent one being the last element. `None` variants indicate that /// the element is younger than five compilations. - temperature: [Option; 5], + temperature: [usize; 5], + /// For how long the element already exists. + age: usize, } impl FramesEntry { /// Construct a new instance. pub fn new(frames: Vec>>, level: usize) -> Self { - let mut temperature = [None; 5]; - temperature[4] = Some(0); - Self { frames, level, temperature } + Self { + frames, + level, + temperature: [0; 5], + age: 1, + } } /// Checks if the cached [`Frame`] is valid for the given regions. @@ -122,8 +126,7 @@ impl FramesEntry { } } - let tmp = self.temperature.get_mut(4).unwrap(); - *tmp = Some(tmp.map_or(1, |x| x + 1)); + self.temperature[4] = self.temperature[4] + 1; Some(self.frames.clone()) } @@ -131,43 +134,35 @@ impl FramesEntry { /// Get the amount of compilation cycles this item has remained in the /// cache. pub fn age(&self) -> usize { - let mut age = 0; - for &temp in self.temperature.iter().rev() { - if temp.is_none() { - break; - } - age += 1; - } - age + self.age } /// Get the amount of consecutive cycles in which this item has not /// been used. pub fn cooldown(&self) -> usize { - let mut age = 0; + let mut cycle = 0; for (i, &temp) in self.temperature.iter().enumerate().rev() { - match temp { - Some(temp) => { - if temp > 0 { - return self.temperature.len() - 1 - i; - } - } - None => { - return age; + if self.age > i { + if temp > 0 { + return self.temperature.len() - 1 - i; } + } else { + return cycle; } - age += 1 + + cycle += 1 } - age + cycle } /// Whether this element was used in the last compilation cycle. pub fn hit(&self) -> bool { - self.temperature.last().unwrap().unwrap_or(0) != 0 + self.temperature.last().unwrap() != &0 } } +/// These constraints describe regions that match them. #[derive(Debug, Copy, Clone, PartialEq)] pub struct Constraints { /// The minimum available length in the region. @@ -218,49 +213,45 @@ impl Constraints { let base = regions.base.to_spec(); let current = regions.current.to_spec(); - let res = current.eq_by(&self.min, |&x, y| y.map_or(true, |y| x.fits(y))) + current.eq_by(&self.min, |&x, y| y.map_or(true, |y| x.fits(y))) && current.eq_by(&self.max, |x, y| y.map_or(true, |y| x < &y)) && current.eq_by(&self.exact, |&x, y| y.map_or(true, |y| x.approx_eq(y))) - && base.eq_by(&self.base, |&x, y| y.map_or(true, |y| x.approx_eq(y))); - - res + && base.eq_by(&self.base, |&x, y| y.map_or(true, |y| x.approx_eq(y))) } - /// Changes all constraints by adding the argument to them if they are set. + /// Changes all constraints by adding the `size` to them if they are `Some`. pub fn mutate(&mut self, size: Size, regions: &Regions) { - for x in std::array::IntoIter::new([ + for spec in std::array::IntoIter::new([ &mut self.min, &mut self.max, &mut self.exact, &mut self.base, ]) { - if let Some(horizontal) = x.horizontal.as_mut() { + if let Some(horizontal) = spec.horizontal.as_mut() { *horizontal += size.width; } - if let Some(vertical) = x.vertical.as_mut() { + if let Some(vertical) = spec.vertical.as_mut() { *vertical += size.height; } } - self.exact = zip(self.exact, regions.current.to_spec(), |_, o| o); - self.base = zip(self.base, regions.base.to_spec(), |_, o| o); + self.exact = override_if_some(self.exact, regions.current.to_spec()); + self.base = override_if_some(self.base, regions.base.to_spec()); } } -fn zip( +fn override_if_some( one: Spec>, other: Spec, - mut f: F, -) -> Spec> -where - F: FnMut(Length, Length) -> Length, -{ +) -> Spec> { Spec { - vertical: one.vertical.map(|r| f(r, other.vertical)), - horizontal: one.horizontal.map(|r| f(r, other.horizontal)), + vertical: one.vertical.map(|_| other.vertical), + horizontal: one.horizontal.map(|_| other.horizontal), } } +/// Carries an item that only applies to certain regions and the constraints +/// that describe these regions. #[derive(Debug, Copy, Clone, PartialEq)] pub struct Constrained { pub item: T, @@ -275,8 +266,14 @@ impl Deref for Constrained { } } +/// Extends length-related options by providing convenience methods for setting +/// minimum and maximum lengths on them, even if they are `None`. pub trait OptionExt { + // Sets `other` as the value if the Option is `None` or if it contains a + // value larger than `other`. fn set_min(&mut self, other: Length); + // Sets `other` as the value if the Option is `None` or if it contains a + // value smaller than `other`. fn set_max(&mut self, other: Length); } diff --git a/src/layout/pad.rs b/src/layout/pad.rs index cfde07192..f886bec86 100644 --- a/src/layout/pad.rs +++ b/src/layout/pad.rs @@ -15,7 +15,7 @@ impl Layout for PadNode { ctx: &mut LayoutContext, regions: &Regions, ) -> Vec>> { - let original = regions.clone(); + let mut original = regions.clone(); let mut regions = regions.map(|size| size - self.padding.resolve(size).size()); let mut frames = self.child.layout(ctx, ®ions); @@ -39,6 +39,7 @@ impl Layout for PadNode { } regions.next(); + original.next(); *Rc::make_mut(&mut frame.item) = new; } frames diff --git a/src/layout/par.rs b/src/layout/par.rs index 40cb84f2d..504981e66 100644 --- a/src/layout/par.rs +++ b/src/layout/par.rs @@ -190,6 +190,7 @@ impl<'a> ParLayouter<'a> { // line cannot be broken up further. if !stack.regions.current.fits(line.size) { if let Some((last_line, last_end)) = last.take() { + // The region must not fit this line for the result to be valid. if !stack.regions.current.width.fits(line.size.width) { stack.constraints.max.horizontal.set_min(line.size.width); } @@ -213,6 +214,9 @@ impl<'a> ParLayouter<'a> { while !stack.regions.current.height.fits(line.size.height) && !stack.regions.in_full_last() { + // Again, the line must not fit. It would if the space taken up + // plus the line height would fit, therefore the constraint + // below. stack.constraints.max.vertical.set_min( stack.full.height - stack.regions.current.height + line.size.height, ); @@ -225,7 +229,7 @@ impl<'a> ParLayouter<'a> { stack.overflowing = true; break; } - + stack.constraints.max.vertical.set_min( stack.full.height - stack.regions.current.height + line.size.height, ); diff --git a/src/layout/stack.rs b/src/layout/stack.rs index 92a1337fd..045a06e1b 100644 --- a/src/layout/stack.rs +++ b/src/layout/stack.rs @@ -62,6 +62,8 @@ struct StackLayouter<'a> { ruler: Align, /// The constraints for the current region. constraints: Constraints, + /// Whether the last region can fit all the remaining content. + overflowing: bool, /// Offset, alignment and frame for all children that fit into the current /// region. The exact positions are not known yet. frames: Vec<(Length, Gen, Rc)>, @@ -92,6 +94,7 @@ impl<'a> StackLayouter<'a> { full, used: Gen::zero(), ruler: Align::Start, + overflowing: false, frames: vec![], finished: vec![], } @@ -142,9 +145,12 @@ impl<'a> StackLayouter<'a> { } // Find a fitting region. - while !self.regions.current.get(self.main).fits(size.main) - && !self.regions.in_full_last() - { + while !self.regions.current.get(self.main).fits(size.main) { + if self.regions.in_full_last() { + self.overflowing = true; + break; + } + self.constraints .max .get_mut(self.main) @@ -203,6 +209,12 @@ impl<'a> StackLayouter<'a> { size = Size::new(width, width / aspect.into_inner()); } + if self.overflowing { + self.constraints.min.vertical = None; + self.constraints.max.vertical = None; + self.constraints.exact = self.full.to_spec().map(Some); + } + let mut output = Frame::new(size, size.height); let mut first = true; diff --git a/tests/ref/layout/stack.png b/tests/ref/layout/stack.png index 684905f73f002286a1ada6935afc9e9f7229a26d..2f190c4d3b3aa3879ddc004667b208eaff3e0663 100644 GIT binary patch literal 325 zcmeAS@N?(olHy`uVBq!ia0y~yU}OPe6BcHmh(%1m6d=VP;1lBd|NsB_uY;Tys%s@n z0mZD+-2MZpkDe}$Ar-gY-o4G);K1X0FuLh>V?<*D_w9P$umuZ#oH#Ux>zTeir|19O zax5$?Evs7oh%iCHp%X1{)3jkcEcBrjEh&}CKyA1{i?Y+CvXJcDQnZNIAG?sM7L}X-XX~d2MeVRHc7(tFdZ^Iv|aJYGI^lDp-Ai9-S;FP T?bB8SdY-}4)z4*}Q$iB};jvB| literal 264 zcmeAS@N?(olHy`uVBq!ia0y~yU}OQZpRq6l$%A*FfjImDJ|V9E|No!=I>>pUx>m9j zP|PaL?LUy(;_2cTQgQ3;xj^0q0|C|z>W!Nln1pm5#0ztMb8fgjDa^6BboV9M03eVM zz2xEu1&L1&{k*Vn0dBA%OY!D@+|mmc7-*@ioVe=NCo@*upjDIWe-OxO5QtcN!3Su2 c;-N#l-d?`Z?^+hs1KrQy>FVdQ&MBb@05Xna!~g&Q diff --git a/tests/typ/layout/stack.typ b/tests/typ/layout/stack.typ index 006a1412e..50ed1eb19 100644 --- a/tests/typ/layout/stack.typ +++ b/tests/typ/layout/stack.typ @@ -7,3 +7,12 @@ rect(3cm, forest), rect(1cm, conifer), ) + +--- + +#let rect(width, color) = rect(width: 1cm, height: 0.4cm, fill: color) +// This stack overflows. +#box(height: 0.5cm, stack( + rect(3cm, forest), + rect(1cm, conifer), +)) diff --git a/tests/typeset.rs b/tests/typeset.rs index 9e0c95966..740d9a030 100644 --- a/tests/typeset.rs +++ b/tests/typeset.rs @@ -15,14 +15,14 @@ use walkdir::WalkDir; use typst::cache::Cache; use typst::color; -use typst::diag::{Diag, DiagSet, Level}; -use typst::eval::{EvalContext, FuncArgs, FuncValue, Scope, Value}; -use typst::exec::State; +use typst::diag::{Diag, DiagSet, Level, Pass}; +use typst::eval::{eval, EvalContext, FuncArgs, FuncValue, Scope, Value}; +use typst::exec::{exec, State}; use typst::geom::{self, Length, Point, Sides, Size}; use typst::image::ImageId; -use typst::layout::{Element, Fill, Frame, Shape, Text}; +use typst::layout::{layout, Element, Fill, Frame, Shape, Text}; use typst::loading::FsLoader; -use typst::parse::{LineMap, Scanner}; +use typst::parse::{parse, LineMap, Scanner}; use typst::syntax::{Location, Pos}; const TYP_DIR: &str = "./typ"; @@ -224,12 +224,22 @@ fn test_part( state.page.size = Size::new(Length::pt(120.0), Length::inf()); state.page.margins = Sides::splat(Some(Length::pt(10.0).into())); - let mut pass = - typst::typeset(loader, cache, Some(src_path), &src, &scope, state.clone()); + let parsed = parse(src); + let evaluated = eval( + loader, + cache, + Some(src_path), + Rc::new(parsed.output), + &scope, + ); + let executed = exec(&evaluated.output.template, state.clone()); + let layouted = layout(loader, cache, &executed.output); - if !compare_ref { - pass.output.clear(); - } + let mut diags = parsed.diags; + diags.extend(evaluated.diags); + diags.extend(executed.diags); + + let mut pass = Pass::new(layouted, diags); let mut ok = true; @@ -274,35 +284,25 @@ fn test_part( cache.layout.turnaround(); - let mut cached_result = - typst::typeset(loader, cache, Some(src_path), &src, &scope, state.clone()); + let cached_result = layout(loader, cache, &executed.output); - if !compare_ref { - cached_result.output.clear(); - } - - let misses: Vec<_> = cache + let misses = cache .layout .frames .iter() .flat_map(|(_, e)| e) .filter(|e| e.level == level && !e.hit() && e.age() == 2) - .collect(); + .count(); - if !misses.is_empty() { + if misses > 0 { ok = false; - let mut miss_count = 0; - for miss in misses { - dbg!(miss.frames.iter().map(|f| f.constraints).collect::>()); - miss_count += 1; - } println!( " Recompilation had {} cache misses on level {} (Subtest {}) ❌", - miss_count, level, i + misses, level, i ); } - if cached_result != pass { + if cached_result != pass.output { ok = false; println!( " Recompilation of subtest {} differs from clean pass ❌", @@ -314,6 +314,10 @@ fn test_part( cache.layout = reference_cache; cache.layout.turnaround(); + if !compare_ref { + pass.output.clear(); + } + (ok, compare_ref, pass.output) } From 6f518c172805b6ac067a692eb8971c7ec1e50608 Mon Sep 17 00:00:00 2001 From: Martin Date: Sun, 27 Jun 2021 18:30:25 +0200 Subject: [PATCH 4/5] Adjust comments Co-authored-by: Laurenz Update tests/typ/layout/stack.typ Co-authored-by: Laurenz --- src/layout/incremental.rs | 2 +- tests/typ/layout/stack.typ | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/layout/incremental.rs b/src/layout/incremental.rs index a3051d0ff..b6a059c5e 100644 --- a/src/layout/incremental.rs +++ b/src/layout/incremental.rs @@ -162,7 +162,7 @@ impl FramesEntry { } } -/// These constraints describe regions that match them. +/// Describe regions that match them. #[derive(Debug, Copy, Clone, PartialEq)] pub struct Constraints { /// The minimum available length in the region. diff --git a/tests/typ/layout/stack.typ b/tests/typ/layout/stack.typ index 50ed1eb19..89e587c73 100644 --- a/tests/typ/layout/stack.typ +++ b/tests/typ/layout/stack.typ @@ -9,9 +9,9 @@ ) --- +// Test overflowing stack. #let rect(width, color) = rect(width: 1cm, height: 0.4cm, fill: color) -// This stack overflows. #box(height: 0.5cm, stack( rect(3cm, forest), rect(1cm, conifer), From e9960b89424ab67e633076ccc9f8c420316b076a Mon Sep 17 00:00:00 2001 From: Martin Haug Date: Sun, 27 Jun 2021 18:57:08 +0200 Subject: [PATCH 5/5] Code review: Reverse temperature --- src/layout/incremental.rs | 67 +++++++++++++++++++++------------------ src/layout/pad.rs | 17 +++++----- src/layout/par.rs | 16 ++++++---- src/layout/stack.rs | 4 +-- tests/typeset.rs | 18 +++++------ 5 files changed, 65 insertions(+), 57 deletions(-) diff --git a/src/layout/incremental.rs b/src/layout/incremental.rs index b6a059c5e..9eda04026 100644 --- a/src/layout/incremental.rs +++ b/src/layout/incremental.rs @@ -26,6 +26,11 @@ impl LayoutCache { self.frames.clear(); } + /// Amount of items in the cache. + pub fn len(&self) -> usize { + self.frames.iter().map(|(_, e)| e.len()).sum() + } + /// Retains all elements for which the closure on the level returns `true`. pub fn retain(&mut self, mut f: F) where @@ -41,9 +46,10 @@ impl LayoutCache { self.age += 1; for entry in self.frames.iter_mut().flat_map(|(_, x)| x.iter_mut()) { for i in 0 .. (entry.temperature.len() - 1) { - entry.temperature[i] = entry.temperature[i + 1]; + entry.temperature[i + 1] = entry.temperature[i]; } - *entry.temperature.last_mut().unwrap() = 0; + entry.temperature[0] = 0; + entry.age += 1; } } @@ -99,12 +105,11 @@ pub struct FramesEntry { pub frames: Vec>>, /// How nested the frame was in the context is was originally appearing in. pub level: usize, - /// How much the element was accessed during the last five compilations, the - /// most recent one being the last element. `None` variants indicate that - /// the element is younger than five compilations. - temperature: [usize; 5], /// For how long the element already exists. age: usize, + /// How much the element was accessed during the last five compilations, the + /// most recent one being the first element. + temperature: [usize; 5], } impl FramesEntry { @@ -113,8 +118,8 @@ impl FramesEntry { Self { frames, level, - temperature: [0; 5], age: 1, + temperature: [0; 5], } } @@ -126,7 +131,7 @@ impl FramesEntry { } } - self.temperature[4] = self.temperature[4] + 1; + self.temperature[0] += 1; Some(self.frames.clone()) } @@ -141,16 +146,11 @@ impl FramesEntry { /// been used. pub fn cooldown(&self) -> usize { let mut cycle = 0; - for (i, &temp) in self.temperature.iter().enumerate().rev() { - if self.age > i { - if temp > 0 { - return self.temperature.len() - 1 - i; - } - } else { + for &temp in &self.temperature[.. self.age] { + if temp > 0 { return cycle; } - - cycle += 1 + cycle += 1; } cycle @@ -158,7 +158,7 @@ impl FramesEntry { /// Whether this element was used in the last compilation cycle. pub fn hit(&self) -> bool { - self.temperature.last().unwrap() != &0 + self.temperature[0] != 0 } } @@ -213,10 +213,10 @@ impl Constraints { let base = regions.base.to_spec(); let current = regions.current.to_spec(); - current.eq_by(&self.min, |&x, y| y.map_or(true, |y| x.fits(y))) + current.eq_by(&self.min, |x, y| y.map_or(true, |y| x.fits(y))) && current.eq_by(&self.max, |x, y| y.map_or(true, |y| x < &y)) - && current.eq_by(&self.exact, |&x, y| y.map_or(true, |y| x.approx_eq(y))) - && base.eq_by(&self.base, |&x, y| y.map_or(true, |y| x.approx_eq(y))) + && current.eq_by(&self.exact, |x, y| y.map_or(true, |y| x.approx_eq(y))) + && base.eq_by(&self.base, |x, y| y.map_or(true, |y| x.approx_eq(y))) } /// Changes all constraints by adding the `size` to them if they are `Some`. @@ -235,18 +235,13 @@ impl Constraints { } } - self.exact = override_if_some(self.exact, regions.current.to_spec()); - self.base = override_if_some(self.base, regions.base.to_spec()); - } -} + let current = regions.current.to_spec(); + let base = regions.base.to_spec(); -fn override_if_some( - one: Spec>, - other: Spec, -) -> Spec> { - Spec { - vertical: one.vertical.map(|_| other.vertical), - horizontal: one.horizontal.map(|_| other.horizontal), + self.exact.horizontal.set_if_some(current.horizontal); + self.exact.vertical.set_if_some(current.vertical); + self.base.horizontal.set_if_some(base.horizontal); + self.base.vertical.set_if_some(base.vertical); } } @@ -272,9 +267,13 @@ pub trait OptionExt { // Sets `other` as the value if the Option is `None` or if it contains a // value larger than `other`. fn set_min(&mut self, other: Length); + // Sets `other` as the value if the Option is `None` or if it contains a // value smaller than `other`. fn set_max(&mut self, other: Length); + + /// Sets `other` as the value if the Option is `Some`. + fn set_if_some(&mut self, other: Length); } impl OptionExt for Option { @@ -291,4 +290,10 @@ impl OptionExt for Option { None => *self = Some(other), } } + + fn set_if_some(&mut self, other: Length) { + if self.is_some() { + *self = Some(other); + } + } } diff --git a/src/layout/pad.rs b/src/layout/pad.rs index f886bec86..9461f3ff2 100644 --- a/src/layout/pad.rs +++ b/src/layout/pad.rs @@ -15,10 +15,11 @@ impl Layout for PadNode { ctx: &mut LayoutContext, regions: &Regions, ) -> Vec>> { - let mut original = regions.clone(); - let mut regions = regions.map(|size| size - self.padding.resolve(size).size()); - - let mut frames = self.child.layout(ctx, ®ions); + let mut regions = regions.clone(); + let mut frames = self.child.layout( + ctx, + ®ions.map(|size| size - self.padding.resolve(size).size()), + ); for frame in &mut frames { let padded = solve(self.padding, frame.size); @@ -29,19 +30,19 @@ impl Layout for PadNode { let prev = std::mem::take(&mut frame.item); new.push_frame(origin, prev); - frame.constraints.mutate(padding.size(), &original); + frame.constraints.mutate(padding.size(), ®ions); if self.padding.left.is_relative() || self.padding.right.is_relative() { - frame.constraints.base.horizontal = Some(original.base.width); + frame.constraints.base.horizontal = Some(regions.base.width); } if self.padding.top.is_relative() || self.padding.bottom.is_relative() { - frame.constraints.base.vertical = Some(original.base.height); + frame.constraints.base.vertical = Some(regions.base.height); } regions.next(); - original.next(); *Rc::make_mut(&mut frame.item) = new; } + frames } } diff --git a/src/layout/par.rs b/src/layout/par.rs index 504981e66..45eefe29f 100644 --- a/src/layout/par.rs +++ b/src/layout/par.rs @@ -217,9 +217,11 @@ impl<'a> ParLayouter<'a> { // Again, the line must not fit. It would if the space taken up // plus the line height would fit, therefore the constraint // below. - stack.constraints.max.vertical.set_min( - stack.full.height - stack.regions.current.height + line.size.height, - ); + stack + .constraints + .max + .vertical + .set_min(stack.size.height + line.size.height); stack.finish_region(ctx); } @@ -230,9 +232,11 @@ impl<'a> ParLayouter<'a> { break; } - stack.constraints.max.vertical.set_min( - stack.full.height - stack.regions.current.height + line.size.height, - ); + stack + .constraints + .max + .vertical + .set_min(stack.size.height + line.size.height); stack.finish_region(ctx); } // If the line does not fit horizontally or we have a mandatory diff --git a/src/layout/stack.rs b/src/layout/stack.rs index 045a06e1b..fa5fd5840 100644 --- a/src/layout/stack.rs +++ b/src/layout/stack.rs @@ -88,12 +88,12 @@ impl<'a> StackLayouter<'a> { Self { stack, main, - constraints: Constraints::new(expand), expand, regions, full, used: Gen::zero(), ruler: Align::Start, + constraints: Constraints::new(expand), overflowing: false, frames: vec![], finished: vec![], @@ -154,7 +154,7 @@ impl<'a> StackLayouter<'a> { self.constraints .max .get_mut(self.main) - .set_min(size.main + self.used.main); + .set_min(self.used.main + size.main); self.finish_region(); } diff --git a/tests/typeset.rs b/tests/typeset.rs index 740d9a030..31da3ce0f 100644 --- a/tests/typeset.rs +++ b/tests/typeset.rs @@ -15,7 +15,7 @@ use walkdir::WalkDir; use typst::cache::Cache; use typst::color; -use typst::diag::{Diag, DiagSet, Level, Pass}; +use typst::diag::{Diag, DiagSet, Level}; use typst::eval::{eval, EvalContext, FuncArgs, FuncValue, Scope, Value}; use typst::exec::{exec, State}; use typst::geom::{self, Length, Point, Sides, Size}; @@ -233,14 +233,12 @@ fn test_part( &scope, ); let executed = exec(&evaluated.output.template, state.clone()); - let layouted = layout(loader, cache, &executed.output); + let mut layouted = layout(loader, cache, &executed.output); let mut diags = parsed.diags; diags.extend(evaluated.diags); diags.extend(executed.diags); - let mut pass = Pass::new(layouted, diags); - let mut ok = true; for panic in &*panics.borrow() { @@ -255,11 +253,11 @@ fn test_part( ok = false; } - if pass.diags != ref_diags { + if diags != ref_diags { println!(" Subtest {} does not match expected diagnostics. ❌", i); ok = false; - for diag in &pass.diags { + for diag in &diags { if !ref_diags.contains(diag) { print!(" Not annotated | "); print_diag(diag, &map, lines); @@ -267,7 +265,7 @@ fn test_part( } for diag in &ref_diags { - if !pass.diags.contains(diag) { + if !diags.contains(diag) { print!(" Not emitted | "); print_diag(diag, &map, lines); } @@ -302,7 +300,7 @@ fn test_part( ); } - if cached_result != pass.output { + if cached_result != layouted { ok = false; println!( " Recompilation of subtest {} differs from clean pass ❌", @@ -315,10 +313,10 @@ fn test_part( cache.layout.turnaround(); if !compare_ref { - pass.output.clear(); + layouted.clear(); } - (ok, compare_ref, pass.output) + (ok, compare_ref, layouted) } fn parse_metadata(src: &str, map: &LineMap) -> (Option, DiagSet) {