Skip to content

/ cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-39434: Improve float __floordiv__ performance and error message #18147

Open
wants to merge 1 commit into
base: master
from
Open

bpo-39434: Improve float __floordiv__ performance and error message #18147

wants to merge 1 commit into from
+42 −31

Conversation

@corona10
Copy link
Member

corona10 commented Jan 23, 2020 •
edited by bedevere-bot

https://bugs.python.org/issue39434
@corona10 corona10 force-pushed the corona10:bpo-39434 branch from d422a3e to 1545084 Jan 23, 2020
@corona10 corona10 removed the DO-NOT-MERGE label Jan 23, 2020
@corona10 corona10 changed the title [WIP] bpo-39434: Add float __floordiv__ fast path bpo-39434: Add float __floordiv__ fast path Jan 23, 2020
@corona10 corona10 requested review from vstinner and mdickinson Jan 23, 2020
@corona10

This comment has been minimized.

Sign in to view
Copy link
Member Author

corona10 commented Jan 23, 2020

@mdickinson, @vstinner
If this proposal is accepted, I am going to add news.d since the error message is changed.
@corona10 corona10 removed the skip news label Jan 23, 2020
@corona10 corona10 changed the title bpo-39434: Add float __floordiv__ fast path bpo-39434: Remove unnecessary logic of float __floordiv__ Jan 23, 2020
@corona10 corona10 force-pushed the corona10:bpo-39434 branch from 1940a5d to b83bba9 Jan 23, 2020
@mdickinson
mdickinson approved these changes Jan 23, 2020
View changes
Copy link
Member

mdickinson left a comment

Neat trick: I was worried about duplicated code (see the issue discussion), but it seems we can rely on the compiler to inline _float_div_mod and then strip out the redundant stuff.

LGTM
@mdickinson

This comment has been minimized.

Sign in to view
Copy link
Member

mdickinson commented Jan 23, 2020

Is there benefit to applying the same approach to float_rem? Apart from the possibility of a performance boost, it looks as though it would actually make the code simpler (removing existing duplication) and safer. And for floats, % is possibly used more often than // (for argument reduction, for example).
@mdickinson

This comment has been minimized.

Sign in to view
Copy link
Member

mdickinson commented Jan 23, 2020

Apart from the possibility of a performance boost,

Sorry, of course that doesn't apply, since float_rem is already special-cased and doesn't go via divmod.
@corona10 corona10 changed the title bpo-39434: Remove unnecessary logic of float __floordiv__ bpo-39434: Improve float __floordiv__ performance and update error message Jan 24, 2020
@corona10 corona10 changed the title bpo-39434: Improve float __floordiv__ performance and update error message bpo-39434: Improve float __floordiv__ performance and error message Jan 24, 2020
@corona10
bpo-39434: Improve float __floordiv__ performance and error message
Loading status checks…
269e8fe
@corona10 corona10 force-pushed the corona10:bpo-39434 branch from b83bba9 to 269e8fe Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdickinson mdickinson

@vstinner vstinner

Assignees
No one assigned
Labels
CLA signed awaiting merge
Projects
None yet
Milestone
No milestone
4 participants
@corona10 @mdickinson @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.