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

[Explorer09]

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]

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]

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]

1. 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);
}

2. 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.

4. 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]

5. 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]

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

LGTM.

What's the behaviour for isNan(inf)?

2. 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).

4. 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.

5. 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]

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.

4. 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]

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.

4. 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]

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]

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]

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.

[BenBE]

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

[Explorer09]

@BenBE

1. 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.)
2. 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]

@BenBE

1. 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. :)

2. 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.

[BenBE]

Looks reasonable.

[natoscott]

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]

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

[natoscott]

Small thing, rest LGTM.

[natoscott]

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!