pgsql: Avoid a performance regression in float overflow/underflow detec - Mailing list pgsql-committers

From Tom Lane
Subject pgsql: Avoid a performance regression in float overflow/underflow detec
Date
Msg-id E1j2JN8-0001fl-Hn@gemulon.postgresql.org
Whole thread Raw
List pgsql-committers
Avoid a performance regression in float overflow/underflow detection.

Commit 6bf0bc842 replaced float.c's CHECKFLOATVAL() macro with static
inline subroutines, but that wasn't too well thought out.  In the original
coding, the unlikely condition (isinf(result) or result == 0) was checked
first, and the inf_is_valid or zero_is_valid condition only afterwards.
The inline-subroutine coding caused that to be swapped around, which is
pretty horrid for performance because (a) in common cases the is_valid
condition is twice as expensive to evaluate (e.g., requiring two isinf()
calls not one) and (b) in common cases the is_valid condition is false,
requiring us to perform the unlikely-condition check anyway.  Net result
is that one isinf() call becomes two or three, resulting in visible
performance loss as reported by Keisuke Kuroda.

The original fix proposal was to revert the replacement of the macro,
but on second thought, that macro was just a bad idea from the beginning:
if anything it's a net negative for readability of the code.  So instead,
let's just open-code all the overflow/underflow tests, being careful to
test the unlikely condition first (and mark it unlikely() to help the
compiler get the point).

Also, rather than having N copies of the actual ereport() calls, collapse
those into out-of-line error subroutines to save some code space.  This
does mean that the error file/line numbers won't be very helpful for
figuring out where the issue really is --- but we'd already burned that
bridge by putting the ereports into static inlines.

In HEAD, check_float[48]_val() are gone altogether.  In v12, leave them
present in float.h but unused in the core code, just in case some
extension is depending on them.

Emre Hasegeli, with some kibitzing from me and Andres Freund

Discussion: https://postgr.es/m/CANDwggLe1Gc1OrRqvPfGE=kM9K0FSfia0hbeFCEmwabhLz95AA@mail.gmail.com

Branch
------
REL_12_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/764a554d6f5e811fca63b9303114af8890802a1b

Modified Files
--------------
src/backend/utils/adt/float.c   | 163 +++++++++++++++++++++++++++++-----------
src/backend/utils/adt/geo_ops.c |   5 +-
src/include/utils/float.h       |  66 ++++++++++------
3 files changed, 165 insertions(+), 69 deletions(-)


pgsql-committers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: pgsql: walreceiver uses a temporary replication slot by default
Next
From: Jeff Davis
Date:
Subject: pgsql: Logical Tape Set: lazily allocate read buffer.