Messages in this thread | | | Date | Tue, 25 May 2021 12:02:52 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH] lib/math/rational.c: Fix divide by zero |
| |
On Mon, May 24, 2021 at 01:17:48PM -0700, Trent Piepho wrote: > On Mon, May 24, 2021 at 3:51 AM Andy Shevchenko > <andriy.shevchenko@intel.com> wrote: > > On Sat, May 22, 2021 at 05:18:06PM -0700, Trent Piepho wrote: > > > > This misses the test cases (*). Please, develop them with Daniel. > > > > *) We usually don't accept changes in the generic libraries without test cases. > > > > Fixes tag? > > Is there a bug report on a tracker? I just got the email from Yigua.
Fixes tag refers to the existing commit that brought the bug. Also you may need to add Reported-by tag since Yigua reported it.
...
> > I think that refactoring may lead us to check first iteration before even going > > into the loop. But it's another story and we may do it later (the algo uses > > I started that, but it had no advantages and some disadvantages. > > Basically, there are three cases: too large, too small & closest to > zero, too small & closest to non-zero. This code can handle those > three cases by adding three branches, if(d1), if(n1), and if(!d1). > The truth values we need already exist at this point the algorithm. > > If it's at the start, then there still needs to be the three branches > for each case. But the values to test must be calculated too. > > What's more, it's possible that the value is exactly representable in > the allowed range. That's actual appears to be the most common use > case, reducing a fraction to lowest terms (*). By putting the tests > in the "terminate because of limits" case, they don't need to happen > when "terminate because exact value find" is the result. If the check > was first, then it would always happen, even if it wouldn't have been > necessary. > > And the time it took to find this bug shows us that out of bounds > inputs are not a common case, so putting that on the hot path by > checking it first at the expense of the reducing to lowest terms path > doesn't make sense.
Thanks for detailed explanation of your view to the current state of the code. As you noticed I am not insisting on refactoring or so, I was rather wondering if it can be done in the future. Still we might need some performance tests.
Daniel, does KUnit have a capability to test performance? Like running test case 1M times or so and calc average (median?) time of execution.
> (*) One could write a reduce to lowest terms function with an easier > interface. It could be a trivial one expression wrapper around > rational_best_approximation(). It could also be a simpler function, > but I think it would still perform the exact same sequence of > divisions and moduli, so it wouldn't really make any difference.
-- With Best Regards, Andy Shevchenko
| |