gh-155526: correct errno handling in complex_abs()

[skirpichev]

• Issue: complex_abs() should call _Py_c_abs() with errno=0 set #155526

[skirpichev]

CC @vstinner

[skirpichev]

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.

Details

# bench.py
import pyperf

z = complex(3.140625, 1.0)

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

[hpkfft]

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

[skirpichev]

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.

[hpkfft]

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

[skirpichev]

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

[hpkfft]

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

[skirpichev]

I decided to use alternative approach, ignoring errno, as proposed in #156145. This is in a separate commit and can be easily reverted.

Edit: this will benefit from new private API in #156694. So, I would appreciate if that pr will be reviewed/merged first.

[vstinner]

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

[vstinner]

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.

[hpkfft]

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

[vstinner]

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