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

[dak]

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-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.

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.

https://codereview.appspot.com/342170043/