make_draw_bezier_boxes: save work if thickness == 0.0

make_draw_bezier_boxes: save work if thickness == 0.0

This drop make_draw_bezier_boxes from 0.73% to 0.44% in the profile for
MSDM

[Dan Eble, 2020-04-24 18:30:08]

I see I'm late to the party with this comment, but what are the error bars on
that 0.29%?  I'm not sure the decreased readability is worth it.

[hanwenn, 2020-04-24 18:44:05]

The percentages between different have looked fairly repeatable, but as a less
invasive measure is to just tweak the final 'if'

[dak, 2020-04-24 18:56:31]

https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral...
lily/stencil-integral.cc:496: break;
I think that is less readable than desirable.  Instead how about putting

if (th == 0.0)
   break;

behind the push?  That makes very obvious that only one point is being pushed
without further cleverness.

Either way you'll be checking thickness twice if its non-zero.  The trivial way
to avoid that would be to simply unfold the loop.  Like

points[DOWN].push_back (scm_transform (trans, curve.control_[0] + DOWN *
normal);
if (th != 0.0)
  points[UP].push_back (scm_transform (trans, curve.control_[0] + UP * normal;

Written like that, it actually becomes far from trivial to see why this
optimisation would be valid, so maybe add a comment explaining it for the sake
of human readers?

[hanwenn, 2020-04-24 21:06:07]

On Fri, Apr 24, 2020 at 8:56 PM <dak@gnu.org> wrote:
>
>
>
https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc
> File lily/stencil-integral.cc (right):
>
>
https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral...
> lily/stencil-integral.cc:496: break;
> I think that is less readable than desirable.  Instead how about putting
>
> if (th == 0.0)
>    break;
>
> behind the push?  That makes very obvious that only one point is being
> pushed without further cleverness.

Yeah, that's better.

> Either way you'll be checking thickness twice if its non-zero.  The
> trivial way to avoid that would be to simply unfold the loop.  Like
>
> points[DOWN].push_back (scm_transform (trans, curve.control_[0] + DOWN *
> normal);
> if (th != 0.0)
>   points[UP].push_back (scm_transform (trans, curve.control_[0] + UP *
> normal;

the UP/DOWN manipulation has been a frequent source of errors in cut &
paste code, so I'd like to keep the loops.

> Written like that, it actually becomes far from trivial to see why this
> optimisation would be valid, so maybe add a comment explaining it for
> the sake of human readers?

The thickness check is cheap. The expensive bit is calculating
directions and bezier points. I added a comment.

-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

[hanwenn, 2020-05-02 22:26:38]

commit fadb710df7ed135d7712ce50fcdfa83a89753213
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Apr 17 20:20:50 2020 +0200

    make_draw_bezier_boxes: save work if thickness == 0.0