Re: Avoid unnecessary copying of Paper_score vectors (issue 342170043 by address@hidden)

[nine . fierce . ballads]

Reviewers: dak,

Message:
On 2018/06/19 06:32:55, dak wrote:

https://codereview.appspot.com/342170043/diff/1/lily/spacing-determine-loose-columns.cc
> File lily/spacing-determine-loose-columns.cc (right):


https://codereview.appspot.com/342170043/diff/1/lily/spacing-determine-loose-columns.cc#newcode255
> lily/spacing-determine-loose-columns.cc:255: cols->swap(newcols);
> Does this actually generate better code when optimizing?

Until you asked, I had assumed it would be better, but I've checked a
reduced example with gcc 4.9.2 at -O3, and it think it supports making
the change.

https://godbolt.org/g/SkE1tV

With the assignment, there is a call to the assignment operator.  That
might require allocating memory and definitely requires copying the
values.

With the swap, that call goes away and the assembly seems simpler in
other ways, but I haven't sifted through it to understand the details.


Description:
https://sourceforge.net/p/testlilyissues/issues/5350/

Please review this at https://codereview.appspot.com/342170043/

Affected files (+21, -19 lines):
  M lily/axis-group-interface.cc
  M lily/constrained-breaking.cc
  M lily/include/paper-score.hh
  M lily/paper-book.cc
  M lily/paper-score.cc
  M lily/spacing-determine-loose-columns.cc
  M lily/system.cc


Index: lily/axis-group-interface.cc
diff --git a/lily/axis-group-interface.cc b/lily/axis-group-interface.cc
index  
564f638841a52014162705d99b41de00c036bf7f..1e1b21cbe8e8af9c619b2964bd2fd55906a0c1c3  
100644
--- a/lily/axis-group-interface.cc
+++ b/lily/axis-group-interface.cc
@@ -196,8 +196,8 @@ Interval
 Axis_group_interface::combine_pure_heights (Grob *me, SCM measure_extents,  
int start, int end)
 {
   Paper_score *ps = get_root_system (me)->paper_score ();
-  vector<vsize> breaks = ps->get_break_indices ();
-  vector<Grob *> cols = ps->get_columns ();
+  vector<vsize> const &breaks = ps->get_break_indices ();
+  vector<Grob *> const &cols = ps->get_columns ();

   Interval ext;
   for (vsize i = 0; i + 1 < breaks.size (); i++)
@@ -229,7 +229,7 @@ Axis_group_interface::adjacent_pure_heights (SCM smob)
   extract_grob_set (me, "pure-relevant-grobs", elts);

   Paper_score *ps = get_root_system (me)->paper_score ();
-  vector<vsize> ranks = ps->get_break_ranks ();
+  vector<vsize> const &ranks = ps->get_break_ranks ();

   vector<Interval> begin_line_heights;
   vector<Interval> mid_line_heights;
Index: lily/constrained-breaking.cc
diff --git a/lily/constrained-breaking.cc b/lily/constrained-breaking.cc
index  
df08d5433ce6cefd38c944448a80c20f0350c8cf..38cd598966ebf38e3cc1c9a52cc2152febf944de  
100644
--- a/lily/constrained-breaking.cc
+++ b/lily/constrained-breaking.cc
@@ -126,8 +126,10 @@ Constrained_breaking::space_line (vsize i, vsize j)
   bool ragged_right = to_boolean (pscore_->layout ()->c_variable  
("ragged-right"));
   bool ragged_last = to_boolean (pscore_->layout ()->c_variable  
("ragged-last"));

-  vector<Grob *> line (all_.begin () + breaks_[i],
-                       all_.begin () + breaks_[j] + 1);
+  // TODO: Unnecessary copy.  Could pass iterators/indices to
+  // get_line_configuration().  What is the real cost?
+  vector<Grob *> const line (all_.begin () + breaks_[i],
+                             all_.begin () + breaks_[j] + 1);
   Interval line_dims = line_dimensions_int (pscore_->layout (), i);
   bool last = j == breaks_.size () - 1;
   bool ragged = ragged_right || (last && ragged_last);
Index: lily/include/paper-score.hh
diff --git a/lily/include/paper-score.hh b/lily/include/paper-score.hh
index  
3d4d5840e1f075481c8d20b060353c7bfca9f633..7d235724f1610a1cb4f46eaba9b889e918b27de5  
100644
--- a/lily/include/paper-score.hh
+++ b/lily/include/paper-score.hh
@@ -44,9 +44,9 @@ public:

   void typeset_system (System *);
   vector<Column_x_positions> calc_breaking ();
-  vector<vsize> get_break_indices () const;
-  vector<vsize> get_break_ranks () const;
-  vector<Grob *> get_columns () const;
+  vector<vsize> const &get_break_indices () const;
+  vector<vsize> const &get_break_ranks () const;
+  vector<Grob *> const &get_columns () const;
   SCM get_paper_systems ();
 protected:
   void find_break_indices () const;
Index: lily/paper-book.cc
diff --git a/lily/paper-book.cc b/lily/paper-book.cc
index  
f1787e99550f2bb09eb001188b69384c69e5f004..449f82eebfb2c00ce7d44d194d49f676226f059b  
100644
--- a/lily/paper-book.cc
+++ b/lily/paper-book.cc
@@ -344,7 +344,7 @@ set_page_permission (SCM sys, SCM symbol, SCM  
permission)
 {
   if (Paper_score *ps = unsmob<Paper_score> (sys))
     {
-      vector<Grob *> cols = ps->get_columns ();
+      vector<Grob *> const &cols = ps->get_columns ();
       if (cols.size ())
         {
           Paper_column *col = dynamic_cast<Paper_column *> (cols.back ());
@@ -388,7 +388,7 @@ set_labels (SCM sys, SCM labels)
 {
   if (Paper_score *ps = unsmob<Paper_score> (sys))
     {
-      vector<Grob *> cols = ps->get_columns ();
+      vector<Grob *> const &cols = ps->get_columns ();
       if (cols.size ())
         {
           Paper_column *col = dynamic_cast<Paper_column *> (cols[0]);
Index: lily/paper-score.cc
diff --git a/lily/paper-score.cc b/lily/paper-score.cc
index  
0cecb7951170d53e022523cdbdb3876493741f79..08fb445ed0d2648d06498a2edba10691eff0c796  
100644
--- a/lily/paper-score.cc
+++ b/lily/paper-score.cc
@@ -83,7 +83,7 @@ Paper_score::find_break_indices () const
     }
 }

-vector<vsize>
+vector<vsize> const &
 Paper_score::get_break_indices () const
 {
   if (break_indices_.empty ())
@@ -91,7 +91,7 @@ Paper_score::get_break_indices () const
   return break_indices_;
 }

-vector<Grob *>
+vector<Grob *> const &
 Paper_score::get_columns () const
 {
   if (cols_.empty ())
@@ -99,7 +99,7 @@ Paper_score::get_columns () const
   return cols_;
 }

-vector<vsize>
+vector<vsize> const &
 Paper_score::get_break_ranks () const
 {
   if (break_ranks_.empty ())
Index: lily/spacing-determine-loose-columns.cc
diff --git a/lily/spacing-determine-loose-columns.cc  
b/lily/spacing-determine-loose-columns.cc
index  
4be23042ad44bbddc007c8879714cd0c201bd35d..df19421e499d1e3bf5e4f51717c1fe65cca91c47  
100644
--- a/lily/spacing-determine-loose-columns.cc
+++ b/lily/spacing-determine-loose-columns.cc
@@ -252,7 +252,7 @@ Spacing_spanner::prune_loose_columns (Grob *me,
         newcols.push_back (c);
     }

-  *cols = newcols;
+  cols->swap(newcols);
 }

 /*
Index: lily/system.cc
diff --git a/lily/system.cc b/lily/system.cc
index  
d8e140d5e984306fad8577260379c0e409d1cfec..fa10575a020f28cf27af83eeed7572791af5cef8  
100644
--- a/lily/system.cc
+++ b/lily/system.cc
@@ -434,7 +434,7 @@ System::break_into_pieces (vector<Column_x_positions>  
const &breaking)
       System *system = clone ();
       system->rank_ = broken_intos_.size ();

-      vector<Grob *> c (breaking[i].cols_);
+      vector<Grob *> const &c (breaking[i].cols_);
       pscore_->typeset_system (system);

       int st = Paper_column::get_rank (c[0]);
@@ -456,7 +456,7 @@ System::break_into_pieces (vector<Column_x_positions>  
const &breaking)
         Collect labels from any loose columns too: theses will be set on
         an empty bar line or a column which is otherwise unused mid-line
       */
-      vector<Grob *> loose (breaking[i].loose_cols_);
+      vector<Grob *> const &loose (breaking[i].loose_cols_);
       for (vsize j = 0; j < loose.size (); j++)
         collect_labels (loose[j], &system_labels);

@@ -932,9 +932,9 @@ System::calc_pure_height (SCM smob, SCM start_scm, SCM  
end_scm)
 Grob *
 System::get_pure_bound (Direction d, int start, int end)
 {
-  vector<vsize> ranks = pscore_->get_break_ranks ();
-  vector<vsize> indices = pscore_->get_break_indices ();
-  vector<Grob *> cols = pscore_->get_columns ();
+  vector<vsize> const &ranks = pscore_->get_break_ranks ();
+  vector<vsize> const &indices = pscore_->get_break_indices ();
+  vector<Grob *> const &cols = pscore_->get_columns ();

   vsize target_rank = (d == LEFT ? start : end);
   vector<vsize>::const_iterator i