gh-155526: correct errno handling in complex_abs() - #155527

There is a faster alternative to this, using hypot directly.

The C standard says:

Each of the functions cabs and carg is specified by a formula in terms of a real function (whose special cases are covered in Annex F):
cabs(x + iy ) = hypot(x, y )
carg(x + iy ) = atan2(y , x)

I got (default ./configure flags, gcc 14.2):

Mean +- std dev: [ref] 259 ns +- 3 ns -> [patch2] 231 ns +- 10 ns: 1.12x faster

with

diff --git a/Objects/complexobject.c b/Objects/complexobject.c
index 3612c2699a5..1554fe4905a 100644
--- a/Objects/complexobject.c
+++ b/Objects/complexobject.c
@@ -796,8 +796,13 @@ static PyObject *
 complex_abs(PyObject *op)
 {
     PyComplexObject *v = _PyComplexObject_CAST(op);
-    double result = _Py_c_abs(v->cval);
-    if (errno == ERANGE) {
+    double result;
+
+    result = hypot(v->cval.real, v->cval.imag);
+    /* Testing FE_OVERFLOW floating-point exception is slow. */
+    if (isfinite(v->cval.real) && isfinite(v->cval.imag)
+        && !isfinite(result))
+    {
         PyErr_SetString(PyExc_OverflowError,
                         "absolute value too large");
         return NULL;

Let me know if you prefer this version. See also #156145.

# bench.py
import pyperf

z = complex(3.140625, 1.0)

runner = pyperf.Runner()
runner.bench_func("abs(z)", abs, z)

The function _Py_c_abs() does special value testing itself (if either real or imag part is inf, return inf even if the other part is NaN), so it does not rely on the platform's math library to do this correctly (as required by Annex F). Can we rely on the platform math library to do this correctly? If so, then the special value testing can be removed from _Py_c_abs().

I wonder how much performance gain came from not checking errno (which hopefully we'll get from merging the enhancement "issue" you noted above) and how much came from removing/skipping the special value testing and relying on the C math library to do it. (Well, I saw 1.07x for the former, but on very different hardware.)

If so, then the special value testing can be removed from _Py_c_abs().

This is soft-deprecated API function. Technically, we could also drop everything here, except for hypot() call and testing it's output value. At price that will break some exotic platform.

As there is no bug, lets keep things here as is.

I saw 1.07x for the former, but on very different hardware.

I got something like this with your patch.

If you're willing to take the risk of breaking abs() on some exotic platform for the huge number of Python developers, you should also take the risk of breaking the C API function _Py_c_abs() on the same exotic platform for, ummm, well, nobody really.
I think there's a reasonable expectation that the C API function returns the same bits as the Python function. I would like to see them kept in sync....

I think there's a reasonable expectation that the C API function returns the same bits as the Python function. I would like to see them kept in sync....

It's already out of sync, e.g. for _Py_c_pow() vs complex_pow().

I would suggest fixing that (as part of #156145 or as a separate PR). Anybody calling _Py_c_pow() (e.g., Numba) should get the same results as a Python developer calling pow(). The latter has an optimization for small integer exponents. Numba should be allowed to benefit from that. (I'm working with Sergey offline to verify the optimization is more accurate (as claimed).)

Can you mention in the comment that errno is not used on purpose, maybe with a reference to gh-155526?

How about following?

We have only one other place, where errno tested for libm's functions. That one removed in #156694.

If that PR will be merged, we can add a generic commentary to this file: that we don't test errno values, coming from the library functions.

No longer setting errno to 0 sounds risky. With this change, multiple cmath function now depends on the current errno value: polar() and isclose().

Since _Py_c_abs() is our custom API, why not change its API to report the error, rather than relying on the global variable errno?

For example, change the API to int _Py_c_abs(Py_complex z, double *result): set *result and return 0 on success, return -1 on error.

Unfortunately, _Py_c_abs() is documented.
An external API should not set errno to zero when the function succeeds. (So, I don't think we should change the documentation to say it does. We should change the implementation.)

The function polar() sets errno = 0 before calling _Py_c_abs(z), so it's OK.
The function isclose() does not read errno, so it's OK.

I suggest: https://github.com/hpkfft/cpython/blob/erange/Objects/complexobject.c#L380-L417
This keeps the documented API, but adds a new function c_abs() for internal use.
If c_abs() is useful in cmathmodule.c, maybe it needs a better name (and, of course, cannot be static).
This can be done as part of #156145

Oh, I forgot that _Py_c_abs() is part of the public C API (but is private).

No longer setting errno to 0 sounds risky.

This is not a part of the documentation and looks as a mistake. Note that other private complex C-API functions don't do this. This also corresponds to libc behavior. The C standard says:

The value of errno in the initial thread is zero at program startup (the initial representation of the object designated by errno in other threads is indeterminate), but is never set to zero by any library function.

and

Thus, a program that uses errno for error checking would set it to zero before a library function call, then inspect it before a subsequent library function call.

I think that rare C-API users adopt above pattern, like we do in polar().

Though, I'm fine with reversion of that part if you aren't OK with arguments above.

If c_abs() is useful in cmathmodule.c, maybe it needs a better name (and, of course, cannot be static).

Yes, it seems that errno-free helpers shows some speedup in simple tests (5-10%). But if we decide to change internal API functions in this way, lets do that more systematically, not just for one function.