GNOME.org
 

[librsvg: 3/5] Turn a PathBuilder into an immutable Path with into_boxed_slice()

commit fc44abd1a8a85b7d8c474260e315cf4c73e4ac01
Author: Federico Mena Quintero <federico gnome org>
Date:   Mon Mar 23 21:32:17 2020 -0600

    Turn a PathBuilder into an immutable Path with into_boxed_slice()
    
    A Path contains a boxed slice of PathCommand, which is the
    shrunk-to-fit Vec<PathCommand> from the PathBuilder.  The boxed slice
    doesn't have the overhead of the capacity value.
    
    For the huge map file in issue #574, this reduces path overhead by
    a nice bit:
    
    Before:
    --------------------------------------------------------------------------------
      n        time(i)         total(B)   useful-heap(B) extra-heap(B)    stacks(B)
    --------------------------------------------------------------------------------
     23 22,751,613,855    1,560,916,408    1,493,746,540    67,169,868            0
    95.70% (1,493,746,540B) (heap allocation functions) malloc/new/new[], --alloc-fns, etc.
    ->43.94% (685,929,024B) 0x49FF0BB: alloc (alloc.rs:84)
    | ->43.94% (685,929,024B) 0x49FF0BB: exchange_malloc (alloc.rs:206)
    |   ->43.94% (685,929,024B) 0x49FF0BB: new<rsvg_internals::element::Element> (boxed.rs:121)
    |   ...
    |
    ->36.05% (562,764,384B) 0x49CC457: realloc (alloc.rs:128)
    | ->36.05% (562,764,384B) 0x49CC457: realloc (alloc.rs:187)
    |   ->36.05% (562,764,384B) 0x49CC457: 
reserve_internal<rsvg_internals::path_builder::PathCommand,alloc::alloc::Global> (raw_vec.rs:693)
    |     ->36.05% (562,764,384B) 0x49CC457: alloc::raw_vec::RawVec<T,A>::reserve (raw_vec.rs:520)
    |       ->22.36% (349,090,560B) 0x49927B0: reserve<rsvg_internals::path_builder::PathCommand> (vec.rs:501)
    |       | ->22.36% (349,090,560B) 0x49927B0: push<rsvg_internals::path_builder::PathCommand> (vec.rs:1150)
    
    After:
    --------------------------------------------------------------------------------
      n        time(i)         total(B)   useful-heap(B) extra-heap(B)    stacks(B)
    --------------------------------------------------------------------------------
     30 22,796,106,012    1,553,581,072    1,329,943,324   223,637,748            0
    85.61% (1,329,943,324B) (heap allocation functions) malloc/new/new[], --alloc-fns, etc.
    ->44.15% (685,929,024B) 0x49FF25B: alloc (alloc.rs:84)
    | ->44.15% (685,929,024B) 0x49FF25B: exchange_malloc (alloc.rs:206)
    |   ->44.15% (685,929,024B) 0x49FF25B: new<rsvg_internals::element::Element> (boxed.rs:121)
    |
    The ones with slack space:
    ->22.53% (350,014,176B) 0x4A23300: realloc (alloc.rs:128)
    | ->22.53% (350,014,176B) 0x4A23300: realloc (alloc.rs:187)
    |   ->22.53% (350,014,176B) 0x4A23300: 
shrink_to_fit<rsvg_internals::path_builder::PathCommand,alloc::alloc::Global> (raw_vec.rs:633)
    |     ->22.53% (350,014,176B) 0x4A23300: shrink_to_fit<rsvg_internals::path_builder::PathCommand> 
(vec.rs:623)
    |       ->22.53% (350,014,176B) 0x4A23300: alloc::vec::Vec<T>::into_boxed_slice (vec.rs:679)
    |         ->22.53% (350,014,176B) 0x4A03410: into_path (path_builder.rs:316)
    |
    | ...
    |
    The ones without slack space:
    ->03.48% (54,029,952B) 0x49CC577: realloc (alloc.rs:128)
    | ->03.48% (54,029,952B) 0x49CC577: realloc (alloc.rs:187)
    |   ->03.48% (54,029,952B) 0x49CC577: 
reserve_internal<rsvg_internals::path_builder::PathCommand,alloc::alloc::Global> (raw_vec.rs:693)
    |     ->03.48% (54,029,952B) 0x49CC577: alloc::raw_vec::RawVec<T,A>::reserve (raw_vec.rs:520)
    |       ->02.98% (46,292,544B) 0x49927E0: reserve<rsvg_internals::path_builder::PathCommand> (vec.rs:501)
    |       | ->02.98% (46,292,544B) 0x49927E0: push<rsvg_internals::path_builder::PathCommand> (vec.rs:1150)
    |       |   ->02.98% (46,292,544B) 0x49927E0: line_to (path_builder.rs:325)

 rsvg_internals/src/drawing_ctx.rs  | 10 ++---
 rsvg_internals/src/marker.rs       | 40 +++++++++---------
 rsvg_internals/src/path_builder.rs | 21 +++++++++-
 rsvg_internals/src/path_parser.rs  |  3 +-
 rsvg_internals/src/shapes.rs       | 84 +++++++++++++++++++++-----------------
 5 files changed, 94 insertions(+), 64 deletions(-)
---
diff --git a/rsvg_internals/src/drawing_ctx.rs b/rsvg_internals/src/drawing_ctx.rs
index dfd138ba..08e32519 100644
--- a/rsvg_internals/src/drawing_ctx.rs
+++ b/rsvg_internals/src/drawing_ctx.rs
@@ -970,19 +970,19 @@ impl DrawingCtx {
 
     pub fn draw_path(
         &mut self,
-        builder: &PathBuilder,
+        path: &Path,
         node: &Node,
         acquired_nodes: &mut AcquiredNodes,
         values: &ComputedValues,
         markers: Markers,
         clipping: bool,
     ) -> Result<BoundingBox, RenderingError> {
-        if !builder.is_empty() {
+        if !path.is_empty() {
             let bbox =
                 self.with_discrete_layer(node, acquired_nodes, values, clipping, &mut |an, dc| {
                     let cr = dc.get_cairo_context();
 
-                    builder.to_cairo(&cr)?;
+                    path.to_cairo(&cr)?;
 
                     if clipping {
                         cr.set_fill_rule(cairo::FillRule::from(values.clip_rule));
@@ -994,8 +994,8 @@ impl DrawingCtx {
                 })?;
 
             if markers == Markers::Yes {
-                marker::render_markers_for_path_builder(
-                    builder,
+                marker::render_markers_for_path(
+                    path.get_path_commands(),
                     self,
                     acquired_nodes,
                     values,
diff --git a/rsvg_internals/src/marker.rs b/rsvg_internals/src/marker.rs
index 6aeb0ee1..ee5e5b11 100644
--- a/rsvg_internals/src/marker.rs
+++ b/rsvg_internals/src/marker.rs
@@ -19,7 +19,7 @@ use crate::iri::IRI;
 use crate::length::*;
 use crate::node::{CascadedValues, Node, NodeBorrow, NodeDraw};
 use crate::parsers::{Parse, ParseValue};
-use crate::path_builder::*;
+use crate::path_builder::{arc_segment, ArcParameterization, CubicBezierCurve, PathCommand};
 use crate::properties::{ComputedValues, ParsedProperty, SpecifiedValue, SpecifiedValues};
 use crate::property_bag::PropertyBag;
 use crate::rect::Rect;
@@ -616,8 +616,8 @@ where
     emit_fn(marker_type, x, y, orient)
 }
 
-pub fn render_markers_for_path_builder(
-    builder: &PathBuilder,
+pub fn render_markers_for_path(
+    path_commands: &[PathCommand],
     draw_ctx: &mut DrawingCtx,
     acquired_nodes: &mut AcquiredNodes,
     values: &ComputedValues,
@@ -640,8 +640,8 @@ pub fn render_markers_for_path_builder(
         return Ok(draw_ctx.empty_bbox());
     }
 
-    emit_markers_for_path_builder(
-        builder,
+    emit_markers_for_path(
+        path_commands,
         draw_ctx.empty_bbox(),
         &mut |marker_type: MarkerType, x: f64, y: f64, computed_angle: Angle| {
             if let &IRI::Resource(ref marker) = match marker_type {
@@ -666,8 +666,8 @@ pub fn render_markers_for_path_builder(
     )
 }
 
-fn emit_markers_for_path_builder<E>(
-    builder: &PathBuilder,
+fn emit_markers_for_path<E>(
+    path_commands: &[PathCommand],
     empty_bbox: BoundingBox,
     emit_fn: &mut E,
 ) -> Result<BoundingBox, RenderingError>
@@ -682,7 +682,7 @@ where
     let mut bbox = empty_bbox;
 
     // Convert the path to a list of segments and bare points
-    let segments = Segments::from(builder.get_path_commands());
+    let segments = Segments::from(path_commands);
 
     let mut subpath_state = SubpathState::NoSubpath;
 
@@ -774,7 +774,7 @@ where
                 .unwrap_or_else(|| Angle::new(0.0));
 
             let angle = {
-                if let PathCommand::ClosePath = builder.get_path_commands()[segments.len()] {
+                if let PathCommand::ClosePath = path_commands[segments.len()] {
                     let outgoing = segments
                         .find_outgoing_angle_forwards(0)
                         .unwrap_or_else(|| Angle::new(0.0));
@@ -857,6 +857,7 @@ mod parser_tests {
 #[cfg(test)]
 mod directionality_tests {
     use super::*;
+    use crate::path_builder::PathBuilder;
 
     // Single open path; the easy case
     fn setup_open_path() -> Segments {
@@ -866,7 +867,7 @@ mod directionality_tests {
         builder.line_to(20.0, 10.0);
         builder.line_to(20.0, 20.0);
 
-        Segments::from(builder.get_path_commands())
+        Segments::from(builder.into_path().get_path_commands())
     }
 
     #[test]
@@ -891,7 +892,7 @@ mod directionality_tests {
         builder.curve_to(50.0, 35.0, 60.0, 60.0, 70.0, 70.0);
         builder.line_to(80.0, 90.0);
 
-        Segments::from(builder.get_path_commands())
+        Segments::from(builder.into_path().get_path_commands())
     }
 
     #[test]
@@ -916,7 +917,7 @@ mod directionality_tests {
         builder.line_to(20.0, 20.0);
         builder.close_path();
 
-        Segments::from(builder.get_path_commands())
+        Segments::from(builder.into_path().get_path_commands())
     }
 
     #[test]
@@ -946,7 +947,7 @@ mod directionality_tests {
         builder.line_to(80.0, 90.0);
         builder.close_path();
 
-        Segments::from(builder.get_path_commands())
+        Segments::from(builder.into_path().get_path_commands())
     }
 
     #[test]
@@ -976,7 +977,7 @@ mod directionality_tests {
 
         builder.line_to(40.0, 30.0);
 
-        Segments::from(builder.get_path_commands())
+        Segments::from(builder.into_path().get_path_commands())
     }
 
     #[test]
@@ -1010,7 +1011,7 @@ mod directionality_tests {
     // builder.move_to (30.0, 30.0);
     // builder.move_to (40.0, 40.0);
     //
-    // Segments::from(&builder)
+    // Segments::from(builder.into_path().get_path_commands())
     // }
     //
     // #[test]
@@ -1105,6 +1106,7 @@ mod directionality_tests {
 #[cfg(test)]
 mod marker_tests {
     use super::*;
+    use crate::path_builder::PathBuilder;
 
     #[test]
     fn emits_for_open_subpath() {
@@ -1116,8 +1118,8 @@ mod marker_tests {
 
         let mut v = Vec::new();
 
-        assert!(emit_markers_for_path_builder(
-            &builder,
+        assert!(emit_markers_for_path(
+            builder.into_path().get_path_commands(),
             BoundingBox::new(),
             &mut |marker_type: MarkerType,
                   x: f64,
@@ -1152,8 +1154,8 @@ mod marker_tests {
 
         let mut v = Vec::new();
 
-        assert!(emit_markers_for_path_builder(
-            &builder,
+        assert!(emit_markers_for_path(
+            builder.into_path().get_path_commands(),
             BoundingBox::new(),
             &mut |marker_type: MarkerType,
                   x: f64,
diff --git a/rsvg_internals/src/path_builder.rs b/rsvg_internals/src/path_builder.rs
index 7129aa87..7da68d3a 100644
--- a/rsvg_internals/src/path_builder.rs
+++ b/rsvg_internals/src/path_builder.rs
@@ -290,16 +290,33 @@ impl PathCommand {
     }
 }
 
+/// Constructs a path out of commands
+///
+/// When you are finished constructing a path builder, turn it into
+/// a `Path` with `into_path`.
 #[derive(Clone, Default)]
 pub struct PathBuilder {
     path_commands: Vec<PathCommand>,
 }
 
+/// An immutable path
+///
+/// This is constructed from a `PathBuilder` once it is finished.
+pub struct Path {
+    path_commands: Box<[PathCommand]>,
+}
+
 impl PathBuilder {
     pub fn new() -> PathBuilder {
         PathBuilder::default()
     }
 
+    pub fn into_path(self) -> Path {
+        Path {
+            path_commands: self.path_commands.into_boxed_slice(),
+        }
+    }
+
     pub fn move_to(&mut self, x: f64, y: f64) {
         self.path_commands.push(PathCommand::MoveTo(x, y));
     }
@@ -343,7 +360,9 @@ impl PathBuilder {
     pub fn close_path(&mut self) {
         self.path_commands.push(PathCommand::ClosePath);
     }
+}
 
+impl Path {
     pub fn get_path_commands(&self) -> &[PathCommand] {
         &self.path_commands
     }
@@ -355,7 +374,7 @@ impl PathBuilder {
     pub fn to_cairo(&self, cr: &cairo::Context) -> Result<(), cairo::Status> {
         assert!(!self.is_empty());
 
-        for s in &self.path_commands {
+        for s in self.path_commands.iter() {
             s.to_cairo(cr);
         }
 
diff --git a/rsvg_internals/src/path_parser.rs b/rsvg_internals/src/path_parser.rs
index 994e40e5..f33ab00f 100644
--- a/rsvg_internals/src/path_parser.rs
+++ b/rsvg_internals/src/path_parser.rs
@@ -976,7 +976,8 @@ mod tests {
         let mut builder = PathBuilder::new();
         let result = parse_path_into_builder(path_str, &mut builder);
 
-        let commands = builder.get_path_commands();
+        let path = builder.into_path();
+        let commands = path.get_path_commands();
 
         assert_eq!(expected_commands, commands);
         assert_eq!(expected_result, result);
diff --git a/rsvg_internals/src/shapes.rs b/rsvg_internals/src/shapes.rs
index 94802917..ba1916df 100644
--- a/rsvg_internals/src/shapes.rs
+++ b/rsvg_internals/src/shapes.rs
@@ -13,7 +13,7 @@ use crate::error::*;
 use crate::length::*;
 use crate::node::{CascadedValues, Node};
 use crate::parsers::{optional_comma, Parse, ParseValue};
-use crate::path_builder::*;
+use crate::path_builder::{LargeArc, Path as SvgPath, PathBuilder, Sweep};
 use crate::path_parser;
 use crate::properties::ComputedValues;
 use crate::property_bag::PropertyBag;
@@ -26,13 +26,13 @@ pub enum Markers {
 }
 
 pub struct Shape {
-    builder: Rc<PathBuilder>,
+    path: Rc<SvgPath>,
     markers: Markers,
 }
 
 impl Shape {
-    fn new(builder: Rc<PathBuilder>, markers: Markers) -> Shape {
-        Shape { builder, markers }
+    fn new(path: Rc<SvgPath>, markers: Markers) -> Shape {
+        Shape { path, markers }
     }
 
     fn draw(
@@ -44,7 +44,7 @@ impl Shape {
         clipping: bool,
     ) -> Result<BoundingBox, RenderingError> {
         draw_ctx.draw_path(
-            &self.builder,
+            &self.path,
             node,
             acquired_nodes,
             values,
@@ -54,12 +54,12 @@ impl Shape {
     }
 }
 
-fn make_ellipse(cx: f64, cy: f64, rx: f64, ry: f64) -> PathBuilder {
+fn make_ellipse(cx: f64, cy: f64, rx: f64, ry: f64) -> SvgPath {
     let mut builder = PathBuilder::new();
 
     // Per the spec, rx and ry must be nonnegative
     if rx <= 0.0 || ry <= 0.0 {
-        return builder;
+        return builder.into_path();
     }
 
     // 4/3 * (1-cos 45°)/sin 45° = 4/3 * sqrt(2) - 1
@@ -107,12 +107,12 @@ fn make_ellipse(cx: f64, cy: f64, rx: f64, ry: f64) -> PathBuilder {
 
     builder.close_path();
 
-    builder
+    builder.into_path()
 }
 
 #[derive(Default)]
 pub struct Path {
-    builder: Option<Rc<PathBuilder>>,
+    path: Option<Rc<SvgPath>>,
 }
 
 impl ElementTrait for Path {
@@ -126,7 +126,7 @@ impl ElementTrait for Path {
 
                     rsvg_log!("could not parse path: {}", e);
                 }
-                self.builder = Some(Rc::new(builder));
+                self.path = Some(Rc::new(builder.into_path()));
             }
         }
 
@@ -141,9 +141,9 @@ impl ElementTrait for Path {
         draw_ctx: &mut DrawingCtx,
         clipping: bool,
     ) -> Result<BoundingBox, RenderingError> {
-        if let Some(builder) = self.builder.as_ref() {
+        if let Some(path) = self.path.as_ref() {
             let values = cascaded.get();
-            Shape::new(builder.clone(), Markers::Yes).draw(
+            Shape::new(path.clone(), Markers::Yes).draw(
                 node,
                 acquired_nodes,
                 values,
@@ -194,7 +194,7 @@ impl Parse for Points {
     }
 }
 
-fn make_poly(points: Option<&Points>, closed: bool) -> PathBuilder {
+fn make_poly(points: Option<&Points>, closed: bool) -> SvgPath {
     let mut builder = PathBuilder::new();
 
     if let Some(points) = points {
@@ -211,7 +211,7 @@ fn make_poly(points: Option<&Points>, closed: bool) -> PathBuilder {
         }
     }
 
-    builder
+    builder.into_path()
 }
 
 #[derive(Default)]
@@ -314,16 +314,18 @@ impl ElementTrait for Line {
         clipping: bool,
     ) -> Result<BoundingBox, RenderingError> {
         let values = cascaded.get();
-        Shape::new(
-            Rc::new(self.make_path_builder(values, draw_ctx)),
-            Markers::Yes,
+        Shape::new(Rc::new(self.make_path(values, draw_ctx)), Markers::Yes).draw(
+            node,
+            acquired_nodes,
+            values,
+            draw_ctx,
+            clipping,
         )
-        .draw(node, acquired_nodes, values, draw_ctx, clipping)
     }
 }
 
 impl Line {
-    fn make_path_builder(&self, values: &ComputedValues, draw_ctx: &mut DrawingCtx) -> PathBuilder {
+    fn make_path(&self, values: &ComputedValues, draw_ctx: &mut DrawingCtx) -> SvgPath {
         let mut builder = PathBuilder::new();
 
         let params = draw_ctx.get_view_params();
@@ -336,7 +338,7 @@ impl Line {
         builder.move_to(x1, y1);
         builder.line_to(x2, y2);
 
-        builder
+        builder.into_path()
     }
 }
 
@@ -392,16 +394,18 @@ impl ElementTrait for Rect {
         clipping: bool,
     ) -> Result<BoundingBox, RenderingError> {
         let values = cascaded.get();
-        Shape::new(
-            Rc::new(self.make_path_builder(values, draw_ctx)),
-            Markers::No,
+        Shape::new(Rc::new(self.make_path(values, draw_ctx)), Markers::No).draw(
+            node,
+            acquired_nodes,
+            values,
+            draw_ctx,
+            clipping,
         )
-        .draw(node, acquired_nodes, values, draw_ctx, clipping)
     }
 }
 
 impl Rect {
-    fn make_path_builder(&self, values: &ComputedValues, draw_ctx: &mut DrawingCtx) -> PathBuilder {
+    fn make_path(&self, values: &ComputedValues, draw_ctx: &mut DrawingCtx) -> SvgPath {
         let params = draw_ctx.get_view_params();
 
         let x = self.x.normalize(values, &params);
@@ -438,12 +442,12 @@ impl Rect {
 
         // Per the spec, w,h must be >= 0
         if w <= 0.0 || h <= 0.0 {
-            return builder;
+            return builder.into_path();
         }
 
         // ... and rx,ry must be nonnegative
         if rx < 0.0 || ry < 0.0 {
-            return builder;
+            return builder.into_path();
         }
 
         let half_w = w / 2.0;
@@ -569,7 +573,7 @@ impl Rect {
             builder.close_path();
         }
 
-        builder
+        builder.into_path()
     }
 }
 
@@ -605,16 +609,18 @@ impl ElementTrait for Circle {
         clipping: bool,
     ) -> Result<BoundingBox, RenderingError> {
         let values = cascaded.get();
-        Shape::new(
-            Rc::new(self.make_path_builder(values, draw_ctx)),
-            Markers::No,
+        Shape::new(Rc::new(self.make_path(values, draw_ctx)), Markers::No).draw(
+            node,
+            acquired_nodes,
+            values,
+            draw_ctx,
+            clipping,
         )
-        .draw(node, acquired_nodes, values, draw_ctx, clipping)
     }
 }
 
 impl Circle {
-    fn make_path_builder(&self, values: &ComputedValues, draw_ctx: &mut DrawingCtx) -> PathBuilder {
+    fn make_path(&self, values: &ComputedValues, draw_ctx: &mut DrawingCtx) -> SvgPath {
         let params = draw_ctx.get_view_params();
 
         let cx = self.cx.normalize(values, &params);
@@ -663,16 +669,18 @@ impl ElementTrait for Ellipse {
         clipping: bool,
     ) -> Result<BoundingBox, RenderingError> {
         let values = cascaded.get();
-        Shape::new(
-            Rc::new(self.make_path_builder(values, draw_ctx)),
-            Markers::No,
+        Shape::new(Rc::new(self.make_path(values, draw_ctx)), Markers::No).draw(
+            node,
+            acquired_nodes,
+            values,
+            draw_ctx,
+            clipping,
         )
-        .draw(node, acquired_nodes, values, draw_ctx, clipping)
     }
 }
 
 impl Ellipse {
-    fn make_path_builder(&self, values: &ComputedValues, draw_ctx: &mut DrawingCtx) -> PathBuilder {
+    fn make_path(&self, values: &ComputedValues, draw_ctx: &mut DrawingCtx) -> SvgPath {
         let params = draw_ctx.get_view_params();
 
         let cx = self.cx.normalize(values, &params);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]