Skip to content

PERF: replace use of isnan once MSVC bug is fixed #23209

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

Closed
chris-b1 opened this issue Oct 17, 2018 · 5 comments · Fixed by #45008
Closed

PERF: replace use of isnan once MSVC bug is fixed #23209

chris-b1 opened this issue Oct 17, 2018 · 5 comments · Fixed by #45008
Labels
Performance Memory or execution speed performance Windows Windows OS
Milestone

Comments

@chris-b1
Copy link
Contributor

ref #23182, #21813

MSVC bug tracker - https://developercommunity.visualstudio.com/content/problem/294290/incorrect-codegen-for-double-equality.html

Once that bug is fixed in a widely available MSVC for python 3 builds we should redefine isnan / notnan to be based on an equality check here.
https://github.com/pandas-dev/pandas/pull/23182/files#diff-467e9a847ab859a085518bc17711fe57R20

Current code works around that MSVC bug by using the libc, isnan, which works, but can't be inlined, so likely has a little perf cost
https://godbolt.org/z/y550lB

@chris-b1 chris-b1 added Performance Memory or execution speed performance Windows Windows OS labels Oct 17, 2018
@jbrockmendel
Copy link
Member

@chris-b1 any idea if this has been addressed upstream?

@chris-b1
Copy link
Contributor Author

chris-b1 commented Nov 7, 2019

That bug has been fixed in the most recent version of MSVC - I'm a little disconnected form our build process to know if that's what were using.

@jbrockmendel
Copy link
Member

most recent version of MSVC

Do you know what that version is? In _libs/src/cmath there is a comment "Place upper bound on this check once a fixed MSVC is released"

jbrockmendel added a commit to jbrockmendel/pandas that referenced this issue Nov 8, 2019
@TomAugspurger
Copy link
Contributor

TomAugspurger commented Nov 8, 2019 via email

@jbrockmendel
Copy link
Member

do we have a way of checking if this is actionable?

@jreback jreback added this to the 1.4 milestone Dec 22, 2021
@lithomas1 lithomas1 modified the milestones: 1.4, 1.5 Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Memory or execution speed performance Windows Windows OS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants