Issue 5350: Avoid unnecessary copying of Paper_score vectors

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

[dak, 2018-06-19 06:32:55]

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

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

[Dan Eble, 2018-06-19 11:37:46]

On 2018/06/19 06:32:55, dak wrote:
>
https://codereview.appspot.com/342170043/diff/1/lily/spacing-determine-loose-...
> File lily/spacing-determine-loose-columns.cc (right):
> 
>
https://codereview.appspot.com/342170043/diff/1/lily/spacing-determine-loose-...
> 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.

[dak, 2018-06-19 12:51:46]

On 2018/06/19 11:37:46, Dan Eble wrote:
> On 2018/06/19 06:32:55, dak wrote:
> >
>
https://codereview.appspot.com/342170043/diff/1/lily/spacing-determine-loose-...
> > File lily/spacing-determine-loose-columns.cc (right):
> > 
> >
>
https://codereview.appspot.com/342170043/diff/1/lily/spacing-determine-loose-...
> > 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.

Ok, I'd expected this to get optimized to a move operator maybe but it doesn't. 
But if you really want to optimize a "pruning" operation like that, basically
the best choice would just be to have two iterators run through the array, one
for reading and one for writing, and only advance the writing iterator for stuff
that should get preserved.  However, this code also accessed `code->at(i-1)` so
one would instead need to copy that element out into some `lastcol` variable in
the loop in case it should reference a column that has been removed (does that
happen?  Should it?).  And then at the end one erases the rest of the array. 
That way you don't even need a second array copy.  That would not just save
copying over the pointers but the creation of the whole array.  But one would
need to be careful that this indeed works as intended because of that
`code->at(i-1)` thing.  What is confusing to me is that there is a lot happening
to the columns that _aren't_ retained.  Is that just for the sake of the at(i-1)
thing or are they still being referenced from somewhere else afterwards?

If you are interested in solving this mystery, be sure to record the gist of it
in comments to save the next person the effort.  If not, I guess the swap is
probably fine.  It's just awkward because it insinuates that something happens
with _both_ of the arrays values when one array just dies right afterwards. 
Maybe erase the array before swapping it?  But I suspect that this may be even
more expensive even though it looks less so.

[Dan Eble, 2018-06-19 22:05:24]

On 2018/06/19 12:51:46, dak wrote:
> If you are interested in solving this mystery, be sure to record the gist of
it
> in comments to save the next person the effort.  If not, I guess the swap is
> probably fine.  It's just awkward because it insinuates that something happens

Solving that mystery diverges too much from what I'm working toward, which is
trying to use Paper_column* instead of Grob* in a slew of places.  This copying
thing is just something I noticed along the way that is so easily addressed that
I'd feel bad ignoring it.

I'll add a comment to the code explaining the swap and suggesting that maybe the
algorithm could be reworked to occur in place.

[dak, 2018-06-19 23:05:18]

On 2018/06/19 22:05:24, Dan Eble wrote:
> On 2018/06/19 12:51:46, dak wrote:
> > If you are interested in solving this mystery, be sure to record the gist of
> it
> > in comments to save the next person the effort.  If not, I guess the swap is
> > probably fine.  It's just awkward because it insinuates that something
happens
> 
> Solving that mystery diverges too much from what I'm working toward, which is
> trying to use Paper_column* instead of Grob* in a slew of places.  This
copying
> thing is just something I noticed along the way that is so easily addressed
that
> I'd feel bad ignoring it.
> 
> I'll add a comment to the code explaining the swap and suggesting that maybe
the
> algorithm could be reworked to occur in place.

Well, don't bother with the swap, it will just cause a merge conflict with issue
5351...

[Dan Eble, 2018-06-19 23:35:52]

On 2018/06/19 23:05:18, dak wrote:
> Well, don't bother with the swap, it will just cause a merge conflict with
issue
> 5351...

OK, I've reverted it locally.  I won't update this review unless someone asks
for it.