Shortcut Rational addition if either operand is zero

Shortcut Rational addition if either operand is zero

In a GProf profile of the Carver MSDM score, before 3.14% of
cumulative time was from Rational::operator+=(). Afterwards, it
represents 1.02% of cumulative time.

[lemzwerg, 2020-04-12 12:40:23]

LGTM.  Maybe you can add a comment w.r.t. speed improvement.

[Dan Eble, 2020-04-12 16:55:06]

LGTM

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc
File flower/rational.cc (right):

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc#ne...
flower/rational.cc:257: if (is_infinity ())
The hazard of reviewing this class is that there's always something to wonder
about.  This time, I wonder about adding +infinity and -infinity together (in
either order).

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc#ne...
flower/rational.cc:261: else if (r.sign_ == 0)
Is this the only micro-optimization you measured?  Other ideas you might wish to
try:

* move the conditions that read this->sign_ together

* are infinities handled first because they are significantly more common than
zeros?  try moving the branches that test for zero before the branches that test
for infinities

* inline a subset of the shortcuts in rational.hh

In any case, noting your findings in a concise comment would help future
maintainers avoid erasing your gains.

[hanwenn, 2020-04-12 18:00:35]

dan's comments

[hanwenn, 2020-04-12 18:01:31]

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc
File flower/rational.cc (right):

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc#ne...
flower/rational.cc:257: if (is_infinity ())
On 2020/04/12 16:55:05, Dan Eble wrote:
> The hazard of reviewing this class is that there's always something to wonder
> about.  This time, I wonder about adding +infinity and -infinity together (in
> either order).

yes. I recommend against doing that :-)

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc#ne...
flower/rational.cc:261: else if (r.sign_ == 0)
On 2020/04/12 16:55:05, Dan Eble wrote:
> Is this the only micro-optimization you measured?  Other ideas you might wish
to
> try:
> 
> * move the conditions that read this->sign_ together
> 
> * are infinities handled first because they are significantly more common than
> zeros?  try moving the branches that test for zero before the branches that
test
> for infinities
> 
> * inline a subset of the shortcuts in rational.hh
> 
> In any case, noting your findings in a concise comment would help future
> maintainers avoid erasing your gains.

It's the only one I investigated.  The rational addition shows up because we add
Moments together in many places. Since those generally have grace == 0, it's
doing a lot of unwarranted multiplication/division.

I'll move the zero check first, because it's more likely than the infinity
check.

[hanwenn, 2020-04-17 09:15:14]

reorder (oops, forgot to upload)

[hanwenn, 2020-04-17 09:25:03]

I pushed this to staging by accident; sorry.

I apparently have no permission to do a force-push.

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc
File flower/rational.cc (right):

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc#ne...
flower/rational.cc:261: else if (r.sign_ == 0)
On 2020/04/12 18:01:31, hanwenn wrote:
> On 2020/04/12 16:55:05, Dan Eble wrote:
> > Is this the only micro-optimization you measured?  Other ideas you might
wish
> to
> > try:
> > 
> > * move the conditions that read this->sign_ together
> > 
> > * are infinities handled first because they are significantly more common
than
> > zeros?  try moving the branches that test for zero before the branches that
> test
> > for infinities
> > 
> > * inline a subset of the shortcuts in rational.hh
> > 
> > In any case, noting your findings in a concise comment would help future
> > maintainers avoid erasing your gains.
> 
> It's the only one I investigated.  The rational addition shows up because we
add
> Moments together in many places. Since those generally have grace == 0, it's
> doing a lot of unwarranted multiplication/division.
> 
> I'll move the zero check first, because it's more likely than the infinity
> check.

Done.

[hahnjo, 2020-04-17 09:49:18]

On 2020/04/17 09:25:03, hanwenn wrote:
> I pushed this to staging by accident; sorry.
> 
> I apparently have no permission to do a force-push.

If you want to, you have to remove :staging and re-create it. However the latest
Patch Set 3 is only an artifact of rebasing and the reordering already happened
with Patch Set 2 and was tested back then. So I would also be fine with the
patch staying in.

(LGTM to get the green badge)

[hanwenn, 2020-04-17 09:58:34]

Oh, you are right. Thanks for checking!

On Fri, Apr 17, 2020 at 11:49 AM <jonas.hahnfeld@gmail.com> wrote:
>
> On 2020/04/17 09:25:03, hanwenn wrote:
> > I pushed this to staging by accident; sorry.
> >
> > I apparently have no permission to do a force-push.
>
> If you want to, you have to remove :staging and re-create it. However
> the latest Patch Set 3 is only an artifact of rebasing and the
> reordering already happened with Patch Set 2 and was tested back then.
> So I would also be fine with the patch staying in.
>
> (LGTM to get the green badge)
>
> https://codereview.appspot.com/551690046/



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

[hanwenn, 2020-05-09 10:41:54]

commit fb98d6be7ed06969a8eeab7b974e8734d8cd21b1
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Sun Apr 12 13:32:01 2020 +0200

    Shortcut Rational addition if either operand is zero