Messages in this thread | | | From | Daniel Latypov <> | Date | Tue, 25 May 2021 10:10:15 -0700 | Subject | Re: [PATCH] lib/math/rational.c: Fix divide by zero |
| |
On Tue, May 25, 2021 at 2:02 AM Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > 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.
No, it does not. It also currently lacks an option/flag for running a test multiple times. So one would have to manually modify the test code itself to handle that right now.
(One non-option is to call `kunit.py execute` in a loop, which will avoid build + config overhead, but it still adds more than we'd find acceptable here).
I don't think this was considered before bc it's unclear what the performance characteristics of UML would be like compared to a more "normal" arch. Brendan's current patchset to add QEMU support into kunit.py makes this a bit better, but still running on a physical machine is still probably safest.
> > > (*) 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 > >
| |