classification
Title: Remove unnecessary logic of float __floordiv__
Type: performance Stage: resolved
Components: Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: corona10 Nosy List: corona10, mark.dickinson, shihai1991, vstinner
Priority: normal Keywords: patch

Created on 2020-01-23 14:31 by corona10, last changed 2020-02-02 13:47 by corona10. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 18147 merged corona10, 2020-01-23 14:35
PR 18311 merged mark.dickinson, 2020-02-02 11:14
Messages (14)
msg360559 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-01-23 14:31
./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
msg360560 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2020-01-23 14:38
Is this worth optimising? Floating-point floor division is a comparatively rare operation.
msg360562 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-01-23 15:16
> 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
msg360563 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-01-23 15:20
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.
msg360565 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2020-01-23 15:25
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.
msg360566 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-01-23 15:29
> (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?
msg360568 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-01-23 15:57
@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?
msg361055 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2020-01-30 13:23
New changeset 8d49f7ceb4f961770ae61fe6a4033c4e61cc3288 by Dong-hee Na in branch 'master':
bpo-39434: Improve float __floordiv__ performance and error message (GH-18147)
https://github.com/python/cpython/commit/8d49f7ceb4f961770ae61fe6a4033c4e61cc3288
msg361056 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2020-01-30 13:24
Thank you!
msg361057 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-30 13:49
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.
msg361060 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2020-01-30 14:00
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.
msg361227 - (view) Author: hai shi (shihai1991) * Date: 2020-02-02 10:53
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.
msg361230 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2020-02-02 11:37
@shihai1991 Good catch! Now fixed.
msg361233 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-02-02 13:47
Thanks good catch :)
History
Date User Action Args
2020-02-02 13:47:12corona10setmessages: + msg361233
2020-02-02 11:37:37mark.dickinsonsetmessages: + msg361230
2020-02-02 11:14:56mark.dickinsonsetpull_requests: + pull_request17687
2020-02-02 10:53:17shihai1991setnosy: + shihai1991
messages: + msg361227
2020-01-30 14:00:57mark.dickinsonsetmessages: + msg361060
2020-01-30 13:49:21vstinnersetmessages: + msg361057
2020-01-30 13:24:28mark.dickinsonsetstatus: open -> closed
resolution: fixed
messages: + msg361056

stage: patch review -> resolved
2020-01-30 13:23:44mark.dickinsonsetmessages: + msg361055
2020-01-23 17:24:58corona10setversions: + Python 3.9
2020-01-23 16:11:01corona10settitle: Add float __floordiv__ fast path -> Remove unnecessary logic of float __floordiv__
2020-01-23 15:57:13corona10setmessages: + msg360568
2020-01-23 15:29:18corona10setmessages: + msg360566
2020-01-23 15:25:21mark.dickinsonsetmessages: + msg360565
2020-01-23 15:20:50corona10setmessages: + msg360563
2020-01-23 15:16:53corona10setnosy: + vstinner
messages: + msg360562
2020-01-23 14:38:24mark.dickinsonsetnosy: + mark.dickinson
messages: + msg360560
2020-01-23 14:35:46corona10setkeywords: + patch
stage: patch review
pull_requests: + pull_request17533
2020-01-23 14:31:48corona10settype: performance
2020-01-23 14:31:39corona10create