gh-102221: speed up math.lcm by swapping numbers

[kevin1kevin1k]

Try to speed up lcm by ensuring a <= b before each (a // g) * b operation.
Some discussions:

• Should long_lcm(res, x) be changed to long_lcm(x, res)? This could save some swapping operations
• Are some more tests or benchmarks needed?
• Currently I re-use g for the swapping; could have introduced another one (maybe r just like in _PyLong_GCD)

• Issue: Optimize math.lcm(*xs) #102221

[ghost]

All commit authors signed the Contributor License Agreement.

[kevin1kevin1k]

Hi @corona10 I am contributing the PR during the PyCon APAC 2023 sprint; please review it if you have time (seems I am not able to assign reviewers). Thanks!

[serhiy-storchaka]

PyObject_RichCompareBool() can fail and set error.

What if arguments are negative?

Is there a practical difference between comparing values of integers and only their sizes?

[kevin1kevin1k]

Oh thanks for the reminder, I would add some checks on the compared result.
On the other hand the comment #102221 (comment) in the issue seems to agree with your point: it suffices to compare the sizes (bits) of them. Let me modify so.

[mdickinson]

The idea seems fine in principle, but I agree with others that we want to compare sizes rather than actual values - and as @serhiy-storchaka notes, the current code doesn't seem like the right solution if arguments could be negative.

So there are really two separate issues here:

• make sure that whatever swapping logic is implemented doesn't depend on signs of the inputs
• decide how fine-grained a comparison we want, and benchmark - do we want to compare absolute values? compare bit counts? compare limb counts? I'd expect that comparing limb counts would be enough.

[serhiy-storchaka]

What is a "limb count"?

I would consider two options:

• compare only sizes (the numbers of digits).
• compare sizes, and if they are equal, compare the values of the highest digit.

And then benchmark.

[mdickinson]

What is a "limb count"?

Sorry, "digit count" would be better terminology for this context. ("limb" is GMP terminology, though I think it's more widespread than that; "digit" has the issue that it's somewhat ambiguous, since some might interpret it to mean with "decimal digit".)

[skirpichev]

@kevin1kevin1k are you interested in addressing reviews?

BTW, to compare sizes you could use private _PyLong_DigitCount() helper. I suggest you to start with this part of Serhiy's comment.

[kevin1kevin1k]

@kevin1kevin1k are you interested in addressing reviews?

BTW, to compare sizes you could use private _PyLong_DigitCount() helper. I suggest you to start with this part of Serhiy's comment.

Sorry to all for the late response! I am willing to continue working on this, and thanks for the pointer to the helper function.

[skirpichev]

@kevin1kevin1k, please avoid force-pushing PR's.

[kevin1kevin1k]

@skirpichev @serhiy-storchaka I've rebased with new changes:

• optimize the order in math_lcm_impl: long_lcm(res, x) -> long_lcm(x, res)
• compare the numbers a and b with _PyLong_DigitCount instead of directly comparing their values

Using the similar profiling code that @pochmann provided in the issue:

1. before (main)
   test_lcm (test.test_math.MathTests.test_lcm) ... 1.67 ms ± 0.03 ms lcm

2. after (swapping long_lcm order + gcd optimization)
   test_lcm (test.test_math.MathTests.test_lcm) ... 1.08 ms ± 0.05 ms lcm

[skirpichev]

I've rebased with new changes

You have asked to not do this. @kevin1kevin1k, please avoid altering commit history next time in any way.

[kevin1kevin1k]

Sorry I did not notice the previous comment on not force pushing, and thanks for the reminders. I will not do it in the future.

[serhiy-storchaka]

Well, it works, and brings speed up. But I tried other algorithm, and it brings larger speed up. Details are in the issue.

[kevin1kevin1k]

Well, it works, and brings speed up. But I tried other algorithm, and it brings larger speed up. Details are in the issue.

Yes I saw the discussion and find it interesting (and educative!). Will defer proceeding here until a conclusion is reached there.

[skirpichev]

@kevin1kevin1k, could you fix merge conflict?

[github-actions]

This PR is stale because it has been open for 30 days with no activity.

[kevin1kevin1k]

@kevin1kevin1k, could you fix merge conflict?

Sorry for the delay. I have merged and resolved the conflict. Now the changes are moved to Modules/mathintegermodule.c.

@serhiy-storchaka is this still relevant per the discussion in #102221?