New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimise PyLong division by 1 or -1 #66691
Comments
The attached patch adds fast paths for PyLong division by 1 and -1, as well as dividing 0 by something. This was found helpful for fractions normalisation, as the GCD that is divided by can often be |1|, but firing up the whole division machinery for this eats a lot of CPU cycles for nothing. There are currently two test failures in test_long.py because dividing a huge number by 1 or -1 no longer raises an OverflowError. This is a behavioural change, but I find it acceptable. If others agree, I'll fix the tests and submit a new patch. |
Perhaps it would be worth to special case multiplying on 0, 1 and -1 and adding 0, 1 and -1 too. |
Any optimization requires a benchmark. What is the speedup? |
I proposed an optimization for "x << 0" (as part of a larger patch to optimize 2 ** x) but the issue was rejected: Mark Dickson wrote (msg217863): |
Attaching a similar patch for long_mul(). |
I gave numbers in ticket bpo-22464. """ I then proposed to move this into the PyLong type in general, rather than letting Fraction itself do it less efficiently. |
@serhiy: moving the fast path into l_divmod() has the disadvantage of making it even more complex because we'd then also want to determine the modulus, but only if requested, and it can be 1, 0 or -1, depending on the second value. Sounds like a lot more if's. |
Combined patch for both mul and div that fixes the return value of long_true_div(), as found by Serhiy, and removes the useless change in long_divrem(), as found by Antoine. Thanks! All test_long.py tests pass now. |
Thanks for the reviews, here's a new patch. |
Sorry, last patch version contained a "use before type check" bug. |
Here is an incremental patch that adds fast paths for adding and subtracting 0. Question: the module calls long_long() in some places (e.g. long_abs()) and thus forces the return type to be exactly a PyLong and not a subtype. My changes use a plain "incref+return input value" in some places. Should they call long_long() on it instead? |
Le 26/09/2014 12:57, Stefan Behnel a écrit :
Ah, yes, they should. The return type should not depend on the input |
Ok, updating both patches. |
I reran the fractions benchmark over the final result and the overall gain turned out to be, well, small. It's a clearly reproducible 2-3% faster. That's not bad for the macro impact of a micro-optimisation, but it's not a clear argument for throwing more code at it either. I'll leave it to you to decide. |
Oh, such small gain and only on one specific benchmark not included still in standard benchmark suite, looks discourage. May be other benchmarks have gain from these changes? |
3% is not enough to justify the change. |
Since Serhiy gave another round of valid feedback, here's an updated patch. |
I callgrinded it again and it confirmed that the gain when doing this inside of long_div() and friends is way lower than doing it right in Fraction.__new__(). It's not safe to do there, though, as "is" tests on integers are generally not a good idea in Python code. (Although it doesn't break anything if it fails, as it's a pure optimisation to avoid useless overhead.) The micro benchmarks with timeit confirm that it's as fast as expected. Large numbers before: $ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' ' -x'
10000000 loops, best of 3: 0.177 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x * -2'
1000000 loops, best of 3: 0.329 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x // -2'
100000 loops, best of 3: 2.8 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x * -1'
1000000 loops, best of 3: 0.329 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x // -1'
100000 loops, best of 3: 2.36 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x * 1'
1000000 loops, best of 3: 0.333 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x // 1'
100000 loops, best of 3: 2.37 usec per loop Patched: $ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' ' -x'
10000000 loops, best of 3: 0.176 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x * -2'
1000000 loops, best of 3: 0.328 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x // -2'
100000 loops, best of 3: 2.8 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x * -1'
10000000 loops, best of 3: 0.177 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x // -1'
10000000 loops, best of 3: 0.178 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x * 1'
10000000 loops, best of 3: 0.0244 usec per loop
$ ./python -m timeit -s 'x = 2**2000 + 3**234 + 5**891 + 7**1234' 'x // 1'
10000000 loops, best of 3: 0.0258 usec per loop Small numbers before: $ ./python -m timeit -s 'x = 5' 'x * -2'
10000000 loops, best of 3: 0.0408 usec per loop
$ ./python -m timeit -s 'x = 5' 'x // -2'
10000000 loops, best of 3: 0.0714 usec per loop
$ ./python -m timeit -s 'x = 5' 'x * -1'
10000000 loops, best of 3: 0.0293 usec per loop
$ ./python -m timeit -s 'x = 5' 'x * 1'
10000000 loops, best of 3: 0.0282 usec per loop
$ ./python -m timeit -s 'x = 5' 'x // 1'
10000000 loops, best of 3: 0.0529 usec per loop
$ ./python -m timeit -s 'x = 5' 'x // -1'
10000000 loops, best of 3: 0.0536 usec per loop Patched: $ ./python -m timeit -s 'x = 5' 'x * -2'
10000000 loops, best of 3: 0.0391 usec per loop
$ ./python -m timeit -s 'x = 5' 'x // -2'
10000000 loops, best of 3: 0.0718 usec per loop
$ ./python -m timeit -s 'x = 5' 'x * -1'
10000000 loops, best of 3: 0.0267 usec per loop
$ ./python -m timeit -s 'x = 5' 'x * 1'
10000000 loops, best of 3: 0.0265 usec per loop
$ ./python -m timeit -s 'x = 5' 'x // 1'
10000000 loops, best of 3: 0.0259 usec per loop
$ ./python -m timeit -s 'x = 5' 'x // -1'
10000000 loops, best of 3: 0.0285 usec per loop Note: we're talking µsecs here, not usually something to worry about. And it's unlikely that other benchmarks see similarly "high" speedups as the one for fractions (due to the relatively high likelihood of the GCD being 1 there). I'm ok with closing this ticket as "won't fix". |
One more comment: I also benchmarked the change in long_true_div() now and found that it's only a minor improvement for large numbers and a *pessimisation* for small numbers: Before: $ ./python -m timeit -s 'x = 5' 'x / -1'
10000000 loops, best of 3: 0.0313 usec per loop
$ ./python -m timeit -s 'x = 5' 'x / 1'
10000000 loops, best of 3: 0.0307 usec per loop
$ ./python -m timeit -s 'x = 2**200 + 3**234 + 5**89 + 7**123' 'x / 1'
10000000 loops, best of 3: 0.101 usec per loop
$ ./python -m timeit -s 'x = 2**200 + 3**234 + 5**89 + 7**123' 'x / -1'
10000000 loops, best of 3: 0.104 usec per loop Patched: $ ./python -m timeit -s 'x = 5' 'x / 1'
10000000 loops, best of 3: 0.0569 usec per loop
$ ./python -m timeit -s 'x = 5' 'x / -1'
10000000 loops, best of 3: 0.0576 usec per loop
$ ./python -m timeit -s 'x = 2**200 + 3**234 + 5**89 + 7**123' 'x / -1'
10000000 loops, best of 3: 0.056 usec per loop
$ ./python -m timeit -s 'x = 2**200 + 3**234 + 5**89 + 7**123' 'x / 1'
10000000 loops, best of 3: 0.056 usec per loop
$ ./python -m timeit -s 'x = 2**200 + 3**234 + 5**89 + 7**123' 'x / -2'
10000000 loops, best of 3: 0.106 usec per loop So, just for completeness, here's the patch without that part, with changes only in l_divmod() and long_mul(), with the timeit results as given in my previous comment. |
From reading the discussion thread, it looks like the consensus is to not apply this set of patches because the speed-up is unfortunately small. Closing as won't-fix; please re-open if someone wishes to pursue this again. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: