Skip to content
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

Replace double/float IsNaN + IsInfinity checks with optimized IsFinite #10690

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

h3xds1nz
Copy link
Member

@h3xds1nz h3xds1nz commented Apr 1, 2025

Description

Several versions ago, IsFinite was introduced as an intrinsic, therefore there's no need to perform two separate checks for the same thing, it simplifies the code, uses less bytes and the codegen is also smaller and more efficient.

I've also cleaned up some of the unused util methods in that domain.

Simple benchmark

Method param Mean [ns] Error [ns] StdDev [ns] Code Size [B] Allocated [B]
Original NaN 0.0056 ns 0.0069 ns 0.0065 ns 50 B -
PR__EDIT NaN 0.0029 ns 0.0046 ns 0.0043 ns 27 B -
Original 5 0.1519 ns 0.0082 ns 0.0073 ns 50 B -
PR__EDIT 5 0.0000 ns 0.0000 ns 0.0000 ns 27 B -
Original Infinity 0.1539 ns 0.0091 ns 0.0085 ns 50 B -
PR__EDIT Infinity 0.0002 ns 0.0006 ns 0.0006 ns 27 B -

There are a few places I didn't remove these checks, that should be done separately. Those are constructors that emit different messages for NaN and Infinity. We should have a discussion on how to approach this. As those are all exception cases, ideal approach is probably to create a non-inlinable throw helper that further then checks IsNaN/IsInfinity in case IsFinite already fails and throws the exception with correct messages. Would reduce the ctor complexity for an uncommon case, though it will start to matter a bit less in .NET 10+ as methods with EH frames will now be inlinable.

P.S. That DoubleValue_Validate from DocumentViewer is the real legacy code gem that should have been preserved I guess.

Customer Impact

Improved performance, decreased allocations.

Regression

No.

Testing

Local build.

Risk

Medium, I was doing all of these replacements manually and thus this is rather error-prone where I could invert a condition badly and I didn't notice it. Review carefully this one please, thank you.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners April 1, 2025 19:33
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Apr 1, 2025
Copy link

codecov bot commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 4.41176% with 65 lines in your changes missing coverage. Please review.

Project coverage is 11.06494%. Comparing base (2ded801) to head (8882fa7).

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #10690         +/-   ##
===================================================
+ Coverage   10.95887%   11.06494%   +0.10607%     
===================================================
  Files           3310        3310                 
  Lines         664667      664495        -172     
  Branches       74667       74627         -40     
===================================================
+ Hits           72840       73526        +686     
+ Misses        590685      589834        -851     
+ Partials        1142        1135          -7     
Flag Coverage Δ
Debug 11.06494% <4.41176%> (+0.10607%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@jizc jizc left a comment

Choose a reason for hiding this comment

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

LGTM! Just one comment/question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants