bpo-45510: Specialize BINARY_SUBTRACT by corona10 · Pull Request #29010 · python/cpython

🤖 New build scheduled with the buildbot fleet by @corona10 for commit fb360a2b562300ac17a10c49b43e0f496f51a0ee 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

🤖 New build scheduled with the buildbot fleet by @corona10 for commit b4608cca7dccaf21358e4da854d0701cbe0d0e01 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

The reason of build bot failure looks same with #29024
So unrelated with this PR.

Sorry about the conflicts. I wanted to get both the CALL_FUNCTION specializations PRs merged.

Sorry about the conflicts. I wanted to get both the CALL_FUNCTION specializations PRs merged.

I can do this all day :)

A marginal speedup
Note that the top three speedups are probably just random variations.

My concern with this change is that it will slow down any subtraction on anything other than pairs of ints and floats.
I'd like to see some microbenchmarks for int - float, float - int, and decimal - decimal.

Note that BINARY_ADD has the same issue. If we can fix that, then we could use the same approach here.

A marginal speedup
Note that the top three speedups are probably just random variations.

My concern with this change is that it will slow down any subtraction on anything other than pairs of ints and floats.
I'd like to see some microbenchmarks for int - float, float - int, and decimal - decimal.

Note that BINARY_ADD has the same issue. If we can fix that, then we could use the same approach here.

I share the same concerns

@markshannon cc @pablogsal @Fidget-Spinner

My concern with this change is that it will slow down any subtraction on anything other than pairs of ints and floats.

I definitely agree with your concern. So do I, At first, I thought that it's okayish since we already have the issue with BINARY_ADD. So if this change is not worth to applied, let's stale the PR until the issue is solved.

I'd like to see some microbenchmarks for int - float, float - int, and decimal - decimal.

Whether merge this PR or not, it's worth measuring such benchmarks, we can get some data from this experiment.
I will share the result after writing some scripts.

@markshannon For your data

Benchmark hidden because not significant (1): float - int

from pyperf import Runner

runner = Runner()

runner.timeit("int - int",
    setup="""
import random
a = random.randint(1, 1000000)
b = random.randint(1, 1000000)
""",
    stmt="""
x = a - b
""")
runner.timeit("float - float",
    setup="""
import random
a = random.uniform(1.5, 3.9)
b = random.uniform(1.5, 3.9)
""",
    stmt="""
x = a - b
"""
    )
runner.timeit("int - float",
    setup="""
import random
a = random.randint(1, 100000)
b = random.uniform(1.5, 3.9)
""",
    stmt="""
x = a - b
"""
    )
runner.timeit("float - int",
    setup="""
import random
a = random.randint(1, 100000)
b = random.uniform(1.5, 3.9)
""",
    stmt="""
x = b - a
"""
    )

runner.timeit("decimal - decimal",
    setup="""
import random
from decimal import Decimal as D
a = D(random.randint(1, 100000))
b = D(random.randint(1, 100000))
""",
    stmt="""
x = a - b
"""
    )

@markshannon

Experiment 2

• concept: If either left or right is float handle as both float.
• BINARY_SUBTRACT_INT + BINARY_SUBTRACT_FLOAT = BINARY_SUBTRACT_FAST
• This implementation is a kind of workaround so you may not want but if we only handle (int, float) variation, it will work.
• Need to handle PyFloat_FromDouble is failed and stats handling is needed(This is just PoC)
• c492437

Benchmark hidden because not significant (1): float - int

Unreliable my VM test with experiment2. :)

Benchmark hidden because not significant (16): float, go, json_loads, logging_silent, nbody, pickle_dict, pickle_pure_python, pidigits, pyflate, richards, scimark_fft, scimark_lu, scimark_monte_carlo, scimark_sparse_mat_mult, sqlite_synth, unpickle_list

Will need reworking once #29482 is merged. Sorry to make you redo this, yet again 🙁

I think the end result will be better though, as we won't need yet another ADAPTIVE instruction.

@markshannon
#29482 looks great approach, I will create the new PR once the #29482 is merged.
Thanks for letting me know.

Should this PR be closed (in favor of #29523)?