Replace isnan() with better comparisons (isgreater(), etc.) #1271

Explorer09 · 2023-07-26T12:36:04Z

The standard isnan() function is defined to never throw FP exceptions even when the argument is a "signaling" NaN. This makes isnan() more expensive than (x != x) expression unless the compiler flag -fno-signaling-nans is given.

Introduce functions isNaN(), isNonnegative(), isPositive(), sumPositiveValues() and compareRealNumbers(), and replace isnan() in htop's codebase with the new functions. These functions utilize isgreater() and isgreaterequal() comparisons, which do not throw FP exceptions on "quiet" NaNs, which htop uses extensively.

With isnan() removed, there is no need to suppress the warning -Wno-c11-extensions in FreeBSD. Remove the code from configure.ac.

BenBE · 2023-07-26T12:44:01Z

Maybe introduce a ispositive and isnegative function for the special cases of isgreater(x, 0.) and isless(x, 0.).

Furthermore you may update configure.ac to check for support for -fno-signaling-nans and set it as appropriate, as that's basically what we assume in the code anyway.

BenBE · 2023-07-26T13:09:53Z

Any chance to avoid the #pragma GCC in configure.ac? You can't guarantee that you're dealing with GCC/LLVM after all, because the requirement is C99 compliant, which technicall includes tcc and icc too …

Explorer09 · 2023-07-26T14:46:29Z

I'm considering introducing 3 inline functions in Macros.h:

// Checks if a floating point value is positive. Returns false on a NaN.
static inline bool isPositive(double x) {
   return isgreater(x, 0.0);
}

// Checks if a floating point value is nonnegative. Returns false on a NaN.
static inline bool isNonNegative(double x) {
   return isgreaterequal(x, 0.0);
}

// Cheaper version of C99 isnan(). Unlike isnan(), this may throw FP exception on a "signaling" NaN.
static inline bool isNan(double x) {
   return !isgreaterequal(x, x);
}

I'm not a fan of overriding CFLAGS specified by the user, but I can add a configure-time check if -fsignaling-nans is set and print a warning message saying the flag is not recommended. What do you say?

Any chance to avoid the #pragma GCC in configure.ac? You can't guarantee that you're dealing with GCC/LLVM after all, because the requirement is C99 compliant, which technicall includes tcc and icc too …

That #pragma was to silence a GCC warning when checking for (x != x) behavior when it comes to NaN. (And I think the warning is a false positive.) The alternative would be to not check for the behavior at all, if we avoid the (x != x) expression everywhere.

I have a question with this commit 0e922d4. I don't know why the isnan checks are needed. Specifically in what cases the CPU percentages will sum to a NaN value?

Explorer09 · 2023-07-26T15:16:42Z

Side note: The CPU temperature code is currently the only part where I have to use (x != x) for checking NaN when isnan() is to be avoided. The temperature (in degrees Celsius) could be negative, and that's why. I could replace with a custom inline function isNan (as mentioned in point 1 above) to avoid (x != x). I don't know if it's worth it to add sanity assertions for (x >= -273.15), so I leave that out for now.

BenBE · 2023-07-26T15:29:59Z

I'm considering introducing 3 inline functions in Macros.h:

LGTM.

What's the behaviour for isNan(inf)?

I'm not a fan of overriding CFLAGS specified by the user, but I can add a configure-time check if -fsignaling-nans is set and print a warning message saying the flag is not recommended. What do you say?

Fine by me, too.

Any chance to avoid the #pragma GCC in configure.ac? You can't guarantee that you're dealing with GCC/LLVM after all, because the requirement is C99 compliant, which technicall includes tcc and icc too …

That #pragma was to silence a GCC warning when checking for (x != x) behavior when it comes to NaN. (And I think the warning is a false positive.) The alternative would be to not check for the behavior at all, if we avoid the (x != x) expression everywhere.

I think we better avoid the ==/!= comparisons on floats and thus don't need to play around with those #pragma as they are inherently non-portable AND that style of comparison usually indicates a bug (thus would raise red flags in a code review).

I have a question with this commit 0e922d4. I don't know why the isnan checks are needed. Specifically in what cases the CPU percentages will sum to a NaN value?

That's an implementation detail you see when looking at the CLAMP macro. Also having that additional check is cleaner IMO.

Side note: The CPU temperature code is currently the only part where I have to use (x != x) for checking NaN when isnan() is to be avoided. The temperature (in degrees Celsius) could be negative, and that's why. I could replace with a custom inline function isNan (as mentioned in point 1 above) to avoid (x != x). I don't know if it's worth it to add sanity assertions for (x >= -273.15), so I leave that out for now.

I think the sanity check for absolute zero is not strictly needed, as negative temperature exists. Apart from that, using isNan from bullet 1 is fine by me.

NB, it actually should be isNaN. ;-)

Explorer09 · 2023-07-26T16:18:24Z

What's the behaviour for isNan(inf)?

Infinity is technically not NaN. It would return false even on the C99 standard isnan function.

The difference between standard isnan and the custom macro is this:

              | isnan | (x != x)  | custom macro
--------------+-------+-----------+-------------
-Wfloat-equal | no    | warn      | no
--------------+-------+-----------+-------------
Finite number | false | false     | false
Infinity      | false | false     | false
NaN (quiet)   | true  | true      | true
SNaN          | true  | FPE; true | FPE; true
(FPE means floating point exception)

              | (x >= x)   | isgreaterequal(x,x)
--------------+------------+--------------------
Finite number | true       | true
Infinity      | true       | true
NaN (quiet)   | FPE; false | false
SNaN          | FPE; false | FPE; false

I think we better avoid the ==/!= comparisons on floats and thus don't need to play around with those #pragma as they are inherently non-portable AND that style of comparison usually indicates a bug (thus would raise red flags in a code review).

Okay. So the inline function isNan would be the way to go.

I have a question with this commit 0e922d4. I don't know why the isnan checks are needed. Specifically in what cases the CPU percentages will sum to a NaN value?

That's an implementation detail you see when looking at the CLAMP macro. Also having that additional check is cleaner IMO.

I didn't get it. When using the CLAMP macro, the second and third operand should never be NaN (there's an assertion for that), and when the first operand is NaN, it throws an FP exception or (if it doesn't trap) simply uses the second (low) operand.

My question was, when could the summing operation result in a NaN? With the code like the following:

percent = v[CPU_METER_NICE] + v[CPU_METER_NORMAL] + v[CPU_METER_KERNEL] + v[CPU_METER_IRQ] + v[CPU_METER_SOFTIRQ]

all the values are supposed to be defined and non-NaN, aren't they?

BenBE · 2023-07-26T17:35:40Z

What's the behaviour for isNan(inf)?

Infinity is technically not NaN. It would return false even on the C99 standard isnan function.

The difference between standard isnan and the custom macro is this:

              | isnan | (x != x)  | custom macro
--------------+-------+-----------+-------------
-Wfloat-equal | no    | warn      | no
--------------+-------+-----------+-------------
Finite number | false | false     | false
Infinity      | false | false     | false
NaN (quiet)   | true  | true      | true
SNaN          | true  | FPE; true | FPE; true
(FPE means floating point exception)

              | (x >= x)   | isgreaterequal(x,x)
--------------+------------+--------------------
Finite number | true       | true
Infinity      | true       | true
NaN (quiet)   | FPE; false | false
SNaN          | FPE; false | FPE; false

Okay, just wanted to make sure, those edge cases are covered.

I think we better avoid the ==/!= comparisons on floats and thus don't need to play around with those #pragma as they are inherently non-portable AND that style of comparison usually indicates a bug (thus would raise red flags in a code review).

Okay. So the inline function isNan would be the way to go.

ACK.

I have a question with this commit 0e922d4. I don't know why the isnan checks are needed. Specifically in what cases the CPU percentages will sum to a NaN value?

That's an implementation detail you see when looking at the CLAMP macro. Also having that additional check is cleaner IMO.

I didn't get it. When using the CLAMP macro, the second and third operand should never be NaN (there's an assertion for that), and when the first operand is NaN, it throws an FP exception or (if it doesn't trap) simply uses the second (low) operand.

My question was, when could the summing operation result in a NaN? With the code like the following:
percent = v[CPU_METER_NICE] + v[CPU_METER_NORMAL] + v[CPU_METER_KERNEL] + v[CPU_METER_IRQ] + v[CPU_METER_SOFTIRQ]
all the values are supposed to be defined and non-NaN, aren't they?

There are some platforms that do not provide all of those values. Thus some of those values can be NaN.

Explorer09 · 2023-07-26T19:44:04Z

There are some platforms that do not provide all of those values. Thus some of those values can be NaN.

And it looks like a bug when I read it. NaNs propagate during addition, and if any value of the v array is NaN the whole result would be NaN.

I was considering a function to be added in XUtils.c, but I'm not sure which is the best name for it:
sumPositiveValues, sumPositiveDoubles, or sumPositiveDoubleValues

double sumPositiveValues(const double* array, size_t count) {
   double sum = 0.0;
   for (size_t i = 0; i < count; i++) {
      if (!isPositive(array[i]))
         continue; // Skip negative and NaN values
      sum += array[i];
      if (!(sum < HUGE_VAL))
         break;
   }
   return sum;
}

BenBE · 2023-07-26T21:21:44Z

I was considering a function to be added in XUtils.c, but I'm not sure which is the best name for it: sumPositiveValues, sumPositiveDoubles, or sumPositiveDoubleValues

sumNonNaN … SCNR

Though sumPositiveValues is probably fine …

Explorer09 · 2023-07-26T22:20:52Z

sumNonNaN … SCNR

This sumPositiveValues function would be used in meter code as well (in my branch, at least) and I really intended it to filter out negative values.

linux/LinuxProcessList.c

linux/LibSensors.c

linux/LinuxProcessList.c

XUtils.c

BenBE · 2023-08-01T08:02:48Z

@Explorer09 Anything left you want to change before we can go ahead with this one?

Explorer09 · 2023-08-01T16:28:42Z

@BenBE

The "delta time" calculation code. When I tried to eliminate the usage of isnan calls, this is the only code I haven't finished changing. The goal was to eliminate division by zeros so that the NaN values can be reserved for "not applicable", "not supported by OS" or "error retrieving said data". (Note: I can defer this to another PR if you guys are in a hurry for merging this PR.)
PCPDynamicColumn_compareByKey(): The float and double values may be compared using compareRealNumbers(), but I am not sure if the ordering is desireble for that part of comparison. Can PCP support code developers look at it?

BenBE · 2023-08-01T21:14:56Z

@BenBE

The "delta time" calculation code. When I tried to eliminate the usage of isnan calls, this is the only code I haven't finished changing. The goal was to eliminate division by zeros so that the NaN values can be reserved for "not applicable", "not supported by OS" or "error retrieving said data". (Note: I can defer this to another PR if you guys are in a hurry for merging this PR.)

How much code does this affect? And is this sufficiently distinct from the existing change to go in a different PR? I mean, if this is basically a code change mostly similar to the other changes we've seen so far, i'm quite inclined to say to keep it in this PR. Though if the changes do end up causing a noticeable difference in behaviour, like they probably will, there's good reason to split them just in case we need to partially undo those changes.

And although I'd prefer to go ahead with this PR soonish™ there's actually no preasure to have it been done by yesterday. ;-) Call is yours, just don't drag it on for too long. :)

PCPDynamicColumn_compareByKey(): The float and double values may be compared using compareRealNumbers(), but I am not sure if the ordering is desireble for that part of comparison. Can PCP support code developers look at it?

This should be best directed at @natoscott and @smalinux, who mostly worked on that part.

It should just take a `PCPProcess*` argument rather than a `Process*`. Signed-off-by: Kang-Che Sung <[email protected]>

In PCPProcess_writeField(), the "n" variable should be size_t type. The "n" parameters of Process_printPercentage() and PCPProcess_printDelay() should be size_t type as well. Signed-off-by: Kang-Che Sung <[email protected]>

* Shorter result message for "assume yes (cross compiling)" * Replace grave accent + apostrophe quoting with just apostrophes. It is expected that Autoconf 2.72 updates the quoting as well and the old style has confused a syntax highlighter (Vim). Signed-off-by: Kang-Che Sung <[email protected]>

BenBE

Looks reasonable.

* Use saturatingSub() when subtracting CPU time and I/O read/write bytes of processes so that the values would never go negative (causing unsigned integer wraparound). * The CPU percentages of processes are now NaN if the time interval between measures (the "delta time") is zero. Make the conditional explicit and avoid division by zero. Previously the percentage values would be 0.0. Note: The saturatingSub() function is not used in cpu_delay_percent, blkio_delay_percent and swapin_delay_percent with a comment added that explains the reason. Signed-off-by: Kang-Che Sung <[email protected]>

The standard isnan() function is defined to never throw FP exceptions even when the argument is a "signaling" NaN. This makes isnan() more expensive than (x != x) expression unless the compiler flag '-fno-signaling-nans' is given. Introduce functions isNaN(), isNonnegative(), isPositive(), sumPositiveValues() and compareRealNumbers(), and replace isnan() in htop's codebase with the new functions. These functions utilize isgreater() and isgreaterequal() comparisons, which do not throw FP exceptions on "quiet" NaNs, which htop uses extensively. With isnan() removed, there is no need to suppress the warning '-Wno-c11-extensions' in FreeBSD. Remove the code from 'configure.ac'. Signed-off-by: Kang-Che Sung <[email protected]>

Add a check of the '__SUPPORT_SNAN__' predefined macro and print a warning message if the compiler defines it. The warning is not printed with '--enable-debug' specified as we assume users know what they are doing. :) Signed-off-by: Kang-Che Sung <[email protected]>

The SPACESHIP_NUMBER() macro does not work well with floating point values that are possible to be NaNs. Change the compare logic of all percentage fields of Process entries to use compareRealNumbers(). Signed-off-by: Kang-Che Sung <[email protected]>

natoscott · 2023-08-17T03:55:24Z

That's a question for @natoscott , but I guess they lack implementation for PCP yet …

@Explorer09 @BenBE yep, thanks - these are leftovers from using Linux headers/code at one point in the PCP platform. We'd need some PCP metrics to provide those netlink stats, but that doesn't exist currently so I'll open a separate PR to drop these fields for now.

Explorer09 · 2023-08-17T04:05:16Z

Just to give a note: This PR is ready for review and merging.

natoscott

Small thing, rest LGTM.

Macros.h

natoscott · 2023-08-17T04:21:12Z

PCPDynamicColumn_compareByKey(): The float and double values may be compared using compareRealNumbers(), but I am not sure if the ordering is desireble for that part of comparison. Can PCP support code developers look at it?

Leave this with me, these can use compareRealNumbers but I need to look more closely at the ordering aspect (any change here can be done subsequently). Thanks!

configure.ac

Explorer09 marked this pull request as draft July 26, 2023 12:37

BenBE added this to the 3.3.0 milestone Jul 26, 2023

BenBE added enhancement Extension or improvement to existing feature code quality ♻️ Code quality enhancement labels Jul 26, 2023

Explorer09 force-pushed the fp-compare branch from 9f7519d to fbf8128 Compare July 26, 2023 12:56

Explorer09 closed this Jul 26, 2023

Explorer09 reopened this Jul 26, 2023

Explorer09 force-pushed the fp-compare branch from fbf8128 to 97f49cc Compare July 26, 2023 22:16

Explorer09 force-pushed the fp-compare branch 3 times, most recently from 98182cb to 88b106c Compare July 27, 2023 09:28

Explorer09 commented Jul 27, 2023

View reviewed changes

linux/LinuxProcessList.c Show resolved Hide resolved

Explorer09 force-pushed the fp-compare branch 5 times, most recently from 7418ce3 to 3e0cb1c Compare July 28, 2023 06:54

BenBE reviewed Jul 28, 2023

View reviewed changes

linux/LibSensors.c Show resolved Hide resolved

Explorer09 force-pushed the fp-compare branch from 3e0cb1c to e9b064a Compare July 28, 2023 20:00

BenBE reviewed Jul 28, 2023

View reviewed changes

linux/LinuxProcessList.c Outdated Show resolved Hide resolved

Explorer09 force-pushed the fp-compare branch 5 times, most recently from 5eaaaef to 7661df8 Compare August 1, 2023 06:11

BenBE reviewed Aug 1, 2023

View reviewed changes

XUtils.c Outdated Show resolved Hide resolved

Explorer09 force-pushed the fp-compare branch from 7661df8 to 1a269a2 Compare August 1, 2023 21:27

Explorer09 mentioned this pull request Aug 1, 2023

Inconsistent behaviors of computing CPU% when time "delta" is zero #1274

Open

Explorer09 marked this pull request as ready for review August 1, 2023 22:42

Explorer09 added 3 commits August 17, 2023 03:45

Modify PCPProcessList_updateInfo() prototype for readability

6e1e383

It should just take a `PCPProcess*` argument rather than a `Process*`. Signed-off-by: Kang-Che Sung <[email protected]>

Minor data type fixes in Process.c and PCPProcess.c

02bd755

In PCPProcess_writeField(), the "n" variable should be size_t type. The "n" parameters of Process_printPercentage() and PCPProcess_printDelay() should be size_t type as well. Signed-off-by: Kang-Che Sung <[email protected]>

Explorer09 force-pushed the fp-compare branch from 1a269a2 to 7958197 Compare August 16, 2023 20:23

BenBE reviewed Aug 16, 2023

View reviewed changes

Explorer09 added 4 commits August 17, 2023 10:35

Explorer09 force-pushed the fp-compare branch from 7958197 to 1987104 Compare August 17, 2023 02:37

natoscott approved these changes Aug 17, 2023

View reviewed changes

Macros.h Show resolved Hide resolved

BenBE merged commit 076b913 into htop-dev:main Aug 18, 2023
11 checks passed

Explorer09 deleted the fp-compare branch August 18, 2023 11:02

Explorer09 mentioned this pull request Oct 26, 2023

Incorporate shared memory in bar text #913

Merged

Explorer09 commented Nov 23, 2023

View reviewed changes

configure.ac Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace isnan() with better comparisons (isgreater(), etc.) #1271

Replace isnan() with better comparisons (isgreater(), etc.) #1271

Explorer09 commented Jul 26, 2023 •

edited

Loading

BenBE commented Jul 26, 2023

BenBE commented Jul 26, 2023

Explorer09 commented Jul 26, 2023

Explorer09 commented Jul 26, 2023 •

edited

Loading

BenBE commented Jul 26, 2023

Explorer09 commented Jul 26, 2023

BenBE commented Jul 26, 2023

Explorer09 commented Jul 26, 2023 •

edited

Loading

BenBE commented Jul 26, 2023

Explorer09 commented Jul 26, 2023

BenBE commented Aug 1, 2023

Explorer09 commented Aug 1, 2023

BenBE commented Aug 1, 2023

BenBE left a comment

natoscott commented Aug 17, 2023

Explorer09 commented Aug 17, 2023

natoscott left a comment

natoscott commented Aug 17, 2023

Replace isnan() with better comparisons (isgreater(), etc.) #1271

Replace isnan() with better comparisons (isgreater(), etc.) #1271

Conversation

Explorer09 commented Jul 26, 2023 • edited Loading

BenBE commented Jul 26, 2023

BenBE commented Jul 26, 2023

Explorer09 commented Jul 26, 2023

Explorer09 commented Jul 26, 2023 • edited Loading

BenBE commented Jul 26, 2023

Explorer09 commented Jul 26, 2023

BenBE commented Jul 26, 2023

Explorer09 commented Jul 26, 2023 • edited Loading

BenBE commented Jul 26, 2023

Explorer09 commented Jul 26, 2023

BenBE commented Aug 1, 2023

Explorer09 commented Aug 1, 2023

BenBE commented Aug 1, 2023

BenBE left a comment

Choose a reason for hiding this comment

natoscott commented Aug 17, 2023

Explorer09 commented Aug 17, 2023

natoscott left a comment

Choose a reason for hiding this comment

natoscott commented Aug 17, 2023

Explorer09 commented Jul 26, 2023 •

edited

Loading

Explorer09 commented Jul 26, 2023 •

edited

Loading

Explorer09 commented Jul 26, 2023 •

edited

Loading