Remove unnecessary logic of float __floordiv__

[corona10]

BPO	39434
Nosy	@mdickinson, @vstinner, @corona10, @shihai1991
PRs	bpo-39434: Improve float __floordiv__ performance and error message #18147 bpo-39434: Fix 5-space indentation and trailing whitespace #18311

^{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:

assignee = 'https://github.com/corona10'
closed_at = <Date 2020-01-30.13:24:28.925>
created_at = <Date 2020-01-23.14:31:39.840>
labels = ['3.9', 'performance']
title = 'Remove unnecessary logic of float __floordiv__'
updated_at = <Date 2020-02-02.13:47:12.852>
user = 'https://github.com/corona10'

bugs.python.org fields:

activity = <Date 2020-02-02.13:47:12.852>
actor = 'corona10'
assignee = 'corona10'
closed = True
closed_date = <Date 2020-01-30.13:24:28.925>
closer = 'mark.dickinson'
components = []
creation = <Date 2020-01-23.14:31:39.840>
creator = 'corona10'
dependencies = []
files = []
hgrepos = []
issue_num = 39434
keywords = ['patch']
message_count = 14.0
messages = ['360559', '360560', '360562', '360563', '360565', '360566', '360568', '361055', '361056', '361057', '361060', '361227', '361230', '361233']
nosy_count = 4.0
nosy_names = ['mark.dickinson', 'vstinner', 'corona10', 'shihai1991']
pr_nums = ['18147', '18311']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue39434'
versions = ['Python 3.9']

[corona10]

./python.exe -m pyperf timeit "a = 3.5" "b = a // 2"
AS-IS: Mean +- std dev: 377 ns +- 4 ns
my patch: Mean +- std dev: 204 ns +- 2 ns

[mdickinson]

Is this worth optimising? Floating-point floor division is a comparatively rare operation.

[corona10]

Is this worth optimizing? Floating-point floor division is a comparatively rare operation.

1. I don't want to say that this should always be optimized.
2. However, this operation is a relatively primitive python operation. I think this optimization is easy to bring and worth it.
3. Besides, the relatively unchanged logic, so the maintenance cost is not expected to be large

[corona10]

And on the other side,

>>> 3.8 // 0.0
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ZeroDivisionError: float divmod()

I think that people expect
ZeroDivisionError: float floor division by zero

not the current message.
I caught this optimization issue during I investigate about the error message.

[mdickinson]

So the risk here is that by adding the floordiv fast path, the division code is duplicated, and that increases the risk of accidentally losing the invariant that a // b is interchangeable with divmod(a, b)[0] (e.g., because someone decides to "fix" the floor float division). That and the usual increased maintenance cost of more code.

Whether the optimization is worth the cost, I'm not sure. My gut feeling is not, but others may have different views.

[corona10]

(e.g., because someone decides to "fix" the floor float division)

Okay, what if we create a common divmod function except for creating a tuple?

[corona10]

@mark.dickinson

I extract the common function.
Now maintainence cost is same as AS-IS.

optimization is still work :)

AS-IS: Mean +- std dev: 360 ns +- 19 ns
TO-BE: Mean +- std dev: 185 ns +- 8 ns

what do you think?

[mdickinson]

New changeset 8d49f7c by Dong-hee Na in branch 'master':
bpo-39434: Improve float __floordiv__ performance and error message (GH-18147)
8d49f7c

[mdickinson]

Thank you!

[vstinner]

Thanks, that's a nice optimization!

I'm surprised that creating a tuple of 2 items, get one item directly into the C structure, and destroy the tuple is so slow (360 ns => 185 ns: 175 ns less). With my FASTCALL optimization on function calls, I recall that avoiding the creation a tuple of N items made function calls around 20 ns faster. Not 175 ns.

[mdickinson]

Victor: I suspect there's some compiler magic going on, too; a good compiler should be able to figure out that half of the div/mod calculation is not being used, and strip it out. That wouldn't have been possible before with the tuple packing and unpacking.

[shihai1991]

Small style question: using 5 spaces in https://github.com/python/cpython/blob/master/Objects/floatobject.c#L655-L664 after PR 18147 merged.
Due to bpo don't receive style quesiton, I commented here.

[mdickinson]

@shihai1991 Good catch! Now fixed.

[corona10]

Thanks good catch :)