Skip to content

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

Merged
mdickinson merged 1 commit into
python:masterfrom
corona10:bpo-39434
Jan 30, 2020
Merged

bpo-39434: Improve float __floordiv__ performance and error message#18147
mdickinson merged 1 commit into
python:masterfrom
corona10:bpo-39434

Conversation

@corona10

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

Copy link
Copy Markdown
Member

@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 commented Jan 23, 2020

Copy link
Copy Markdown
Member Author

@mdickinson, @vstinner
If this proposal is accepted, I am going to add news.d since the error message is changed.

@corona10 corona10 changed the title bpo-39434: Add float __floordiv__ fast path bpo-39434: Remove unnecessary logic of float __floordiv__ Jan 23, 2020
mdickinson
mdickinson approved these changes Jan 23, 2020
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mdickinson commented Jan 23, 2020

Copy link
Copy Markdown
Member

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

mdickinson commented Jan 23, 2020

Copy link
Copy Markdown
Member

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
corona10 requested a review from pablogsal January 27, 2020 17:41
@corona10

corona10 commented Jan 28, 2020

Copy link
Copy Markdown
Member Author

@vstinner @pablogsal If you don't mind can you please take a look? :)

@mdickinson

mdickinson commented Jan 30, 2020

Copy link
Copy Markdown
Member

I've done some manual testing as well as reviewing. Still LGTM.

@mdickinson
mdickinson merged commit 8d49f7c into python:master Jan 30, 2020
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@corona10 @shihai1991
bpo-39434: Improve float __floordiv__ performance and error message (p…
ffe52cc 
…ythonGH-18147)
@mdickinson mdickinson mentioned this pull request Feb 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner Awaiting requested review from vstinner
@pablogsal pablogsal Awaiting requested review from pablogsal
1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@corona10 @mdickinson @the-knights-who-say-ni @bedevere-bot