Thanks Nicolas! Also for this elaborate commit message. :-)

Best,
Marcel
Am 28.11.2021 23:16:00 schrieb Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com>:
Note: there might be a minor 2 to 3% penalty for small fractions like 5/3
because this change introduces a super send in the path to Fraction class>>numerator:denominator:
but IMO, it's not worth the code duplication.


Le dim. 28 nov. 2021 à 22:44, <commits at source.squeak.org [mailto:commits at source.squeak.org]> a écrit :

Nicolas Cellier uploaded a new version of Kernel to project The Trunk:
http://source.squeak.org/trunk/Kernel-nice.1428.mcz [http://source.squeak.org/trunk/Kernel-nice.1428.mcz]

==================== Summary ====================

Name: Kernel-nice.1428
Author: nice
Time: 28 November 2021, 10:44:44.154815 pm
UUID: a8a214a1-7879-4782-8dd7-910f183fda65
Ancestors: Kernel-nice.1427

Simplify code for integer division, and marginally fast up some edge case like (SmallInteger / LargeInteger)

Discussion:
Integer was trying to handle case of exact division (Integer / Integer) by performing a quorem operation (digitDiv:neg:), in the hope that avoiding the creation of an unecessary Fraction intermediate would fast things up.

But it appears that digitDiv:neg: does no good nowadays.
- primitive 10 for SmallInteger/SmallInteger case,
- primitive 30 for Integer/Integer case (up to 64 bits)
already handle exact division (their sole purpose if they exist!).

If 10 & 30 are not implemented (they are optional), probably we do not care that much about such micro optimization anyway.

in mixed cases:
- (SmallInteger / LargeInteger), division cannot be exact - except for 0 receiver, so we're just wasting time

Anyway, digitDiv:neg: creates a LargeInteger object for each SmallInteger receiver/argument, and an Array for the resulting quotient and remainder, so in term of creating intermediate objects, it's not much better.
It is also pure waste of time if division is inexact - and division tend to not be cheap for LargeInteger.
Also, gcd: algorithm used in Fraction>>#reduced performs well enough in the case of exact division.

While at it, only handle zero divide in Integer/Integer case.
Let further dispatching handle the case for mixed Integer/Number arithmetic (Integer shall mind its own business).
Note that Fraction>>#setNumerator:denominator: already signal the ZeroDivide, but would not resume: correctly (we send #reduced to the result, which assumes that it is a Fraction)

Warning: benchmarking and understanding the marginal fast-up is not easy, because we have:
- a special bytecode for division
- special primitive for 64 bits integer range
- jit for some of those primitives
that's many cases.

Thanks to https://github.com/pharo-project/pharo/pull/10385 [https://github.com/pharo-project/pharo/pull/10385] for asking.

=============== Diff against Kernel-nice.1427 ===============

Item was changed:
  ----- Method: Integer>>/ (in category 'arithmetic') -----
  / aNumber
        "Refer to the comment in Number / "
-       | quoRem |
        aNumber isInteger ifTrue:
+               [aNumber isZero
+                       ifTrue: [^(ZeroDivide dividend: self) signal].
+               ^ (Fraction numerator: self denominator: aNumber) reduced].
-               [quoRem := self divideByInteger: aNumber.
-               (quoRem at: 2) = 0
-                       ifTrue: [^ (quoRem at: 1) normalize]
-                       ifFalse: [^ (Fraction numerator: self denominator: aNumber) reduced]].
        ^ aNumber adaptToInteger: self andSend: #/!

Item was changed:
  ----- Method: SmallInteger>>/ (in category 'arithmetic') -----
  / aNumber
        "Primitive. This primitive (for /) divides the receiver by the argument
        and returns the result if the division is exact. Fail if the result is not a
        whole integer. Fail if the argument is 0 or is not a SmallInteger. Optional.
        No Lookup. See Object documentation whatIsAPrimitive."

        <primitive: 10>
+       ^super / aNumber!
-       aNumber isZero ifTrue: [^(ZeroDivide dividend: self) signal].
-       ^(aNumber isMemberOf: SmallInteger)
-               ifTrue: [(Fraction numerator: self denominator: aNumber) reduced]
-               ifFalse: [super / aNumber]!


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20211129/d29953bd/attachment.html>