Thank you very much everyone.
Improvement was confirmed even if PG12_STABLE was built with gcc 4.8.5.
* PG_12_STABLE
* gcc 4.8.5
postgres=# EXPLAIN (ANALYZE on, VERBOSE on, BUFFERS on)
select (2 * a) , (2 * b) , (2 * c), (2 * d), (2 * e)
from realtest;
QUERY PLAN
On Fri, Feb 14, 2020 at 3:47 AM Andres Freund wrote:
> On 2020-02-13 13:40:43 -0500, Tom Lane wrote:
> > ... and pushed. One other change I made beyond those suggested
> > was to push the zero-divide ereport's out-of-line as well.
>
> Thanks!
Thank you all.
I repeated some of the tests I did
Hi,
On 2020-02-13 13:40:43 -0500, Tom Lane wrote:
> ... and pushed. One other change I made beyond those suggested
> was to push the zero-divide ereport's out-of-line as well.
Thanks!
> I did not do anything about adding unlikely() calls around the
> unrelated isinf tests in float.c. That
... and pushed. One other change I made beyond those suggested
was to push the zero-divide ereport's out-of-line as well.
I did not do anything about adding unlikely() calls around the
unrelated isinf tests in float.c. That seemed to me to be a separate
matter, and I'm not quite convinced it'd
Andres Freund writes:
> On 2020-02-13 16:25:25 +, Emre Hasegeli wrote:
>> And also this commit is changing the usage of unlikely() to cover
>> the whole condition. Using it only for the result is not semantically
>> correct. It is more than likely for the result to be infinite when
>> the
Andres Freund writes:
> On February 13, 2020 8:30:45 AM PST, Tom Lane wrote:
>> I see some minor things I don't like here, eg float_*flow_error()
>> need some documentation as to why they exist. But I'll review,
>> fix those things up and then push.
> Would be good to mark them noreturn too.
Hi,
On 2020-02-13 16:25:25 +, Emre Hasegeli wrote:
> And also this commit is changing the usage of unlikely() to cover
> the whole condition. Using it only for the result is not semantically
> correct. It is more than likely for the result to be infinite when
> the input is, or it to be 0
Hi,
On February 13, 2020 8:30:45 AM PST, Tom Lane wrote:
>Emre Hasegeli writes:
Cool. Emre, any chance you could write a patch along those lines?
>
>> How about the one attached?
>
>I see some minor things I don't like here, eg float_*flow_error()
>need some documentation as to why they
Emre Hasegeli writes:
>>> Cool. Emre, any chance you could write a patch along those lines?
> How about the one attached?
I see some minor things I don't like here, eg float_*flow_error()
need some documentation as to why they exist. But I'll review,
fix those things up and then push.
> > > > For most places it'd probably end up being easier to read and to
> > > > optimize if we just wrote them as
> > > > if (unlikely(isinf(result)) && !isinf(arg))
> > > > float_overflow_error();
> > > > and when needed added a
> > > > else if (unlikely(result == 0) && arg1 != 0.0)
> > > >
> > > For most places it'd probably end up being easier to read and to
> > > optimize if we just wrote them as
> > > if (unlikely(isinf(result)) && !isinf(arg))
> > > float_overflow_error();
> > > and when needed added a
> > > else if (unlikely(result == 0) && arg1 != 0.0)
> > >
Andres Freund writes:
> I'm inclined that we should backpatch that, and just leave the inline
> function (without in core callers) in place in 12?
Yeah, we can't remove the inline function in 12. But we don't have
to use it.
regards, tom lane
Hi,
On 2020-02-12 14:18:30 -0500, Tom Lane wrote:
> Andres Freund writes:
> > I do wonder if we're just punching ourselves in the face with the
> > signature of these checks. Part of the problem here really comes from
> > using the same function to handle a number of different checks.
>
> Yeah,
Andres Freund writes:
> I do wonder if we're just punching ourselves in the face with the
> signature of these checks. Part of the problem here really comes from
> using the same function to handle a number of different checks.
Yeah, I've thought that too. It's *far* from clear that this thing
Hi,
On 2020-02-12 13:15:22 -0500, Tom Lane wrote:
> Andres Freund writes:
> > I'd just rename the macro to the name of the inline function. No need to
> > have a verbose change in all callsites just to update the name imo.
>
> +1, that's what I had in mind too. That does suggest though that we
Andres Freund writes:
> I'd just rename the macro to the name of the inline function. No need to
> have a verbose change in all callsites just to update the name imo.
+1, that's what I had in mind too. That does suggest though that we
ought to make sure the macro has single-eval behavior, so
On 2020-02-12 17:49:14 +, Emre Hasegeli wrote:
> > Nor do I see how it's going to be ok to just rename the function in a
> > stable branch.
>
> I'll post another version to keep them around.
I'd just rename the macro to the name of the inline function. No need to
have a verbose change in all
> Wait, no. Didn't we get to the point that we figured out that the
> primary issue is the reversal of the order of what is checked is the
> primary problem, rather than the macro/inline piece?
Reversal of the order makes a small or no difference. The
macro/inline change causes the real slowdown
Hi,
On 2020-02-12 11:54:13 +, Emre Hasegeli wrote:
> From fb5052b869255ef9465b1de92e84b2fb66dd6eb3 Mon Sep 17 00:00:00 2001
> From: Emre Hasegeli
> Date: Fri, 7 Feb 2020 10:27:25 +
> Subject: [PATCH] Bring back CHECKFLOATVAL() macro
>
> The inline functions added by 6bf0bc842b caused
> Should we update the same macro in contrib/btree_gist/btree_utils_num.h too?
I posted another version incorporating this.
> But the comment does not explain that this test has to be in that
> order, or the compiler will for non-constant arguments evalute
> the (now) right-side first. E.g. if I understand this correctly:
>
> + if (!(zero_is_valid) && unlikely((val) == 0.0)
>
> would have the same problem of
On Fri, Feb 7, 2020 at 11:43 PM Emre Hasegeli wrote:
> > > The patch looks unduly invasive to me, but I think that it might be
> > > right that we should go back to a macro-based implementation, because
> > > otherwise we don't have a good way to be certain that the function
> > > parameter won't
On Sat, Feb 8, 2020 at 3:13 AM Andres Freund wrote:
> On 2020-02-07 17:17:21 +0900, Amit Langote wrote:
> > I did some tests using two relatively recent compilers: gcc 8 and
> > clang-7 and here are the results:
>
> Hm, these very much look like they've been done in an unoptimized build?
>
> >
Hi,
On 2020-02-07 17:17:21 +0900, Amit Langote wrote:
> I did some tests using two relatively recent compilers: gcc 8 and
> clang-7 and here are the results:
Hm, these very much look like they've been done in an unoptimized build?
> 40.62% postgres postgres [.] ExecInterpExpr
>
Moin,
On 2020-02-07 15:42, Emre Hasegeli wrote:
> The patch looks unduly invasive to me, but I think that it might be
> right that we should go back to a macro-based implementation, because
> otherwise we don't have a good way to be certain that the function
> parameter won't get evaluated
> > The patch looks unduly invasive to me, but I think that it might be
> > right that we should go back to a macro-based implementation, because
> > otherwise we don't have a good way to be certain that the function
> > parameter won't get evaluated first.
>
> I'd first like to see some actual
> Fwiw, also tried the patch that Kuroda-san had posted yesterday.
I run the same test case too:
clang version 7.0.0:
HEAD 2548.119 ms
with patch 2320.974 ms
clang version 8.0.0:
HEAD 2431.766 ms
with patch 2419.439 ms
clang version 9.0.0:
HEAD 2477.493 ms
with patch 2365.509 ms
gcc
Fwiw, also tried the patch that Kuroda-san had posted yesterday.
On Fri, Feb 7, 2020 at 5:17 PM Amit Langote wrote:
> Latency and profiling results:
>
> gcc 8 (gcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3))
>
>
> 11.6
>
> latency average = 463.968 ms
>
> 40.62% postgres postgres
On Fri, Feb 7, 2020 at 4:54 PM Andres Freund wrote:
> On February 6, 2020 11:42:30 PM PST, keisuke kuroda
> wrote:
> >Hi,
> >
> >I have been testing with newer compiler (clang-7)
> >and result is a bit different at least with clang-7.
> >Compiling PG 12.1 (even without patch) with clang-7
>
Hi,
On February 6, 2020 11:42:30 PM PST, keisuke kuroda
wrote:
>Hi,
>
>I have been testing with newer compiler (clang-7)
>and result is a bit different at least with clang-7.
>Compiling PG 12.1 (even without patch) with clang-7
>results in __isinf() no longer being a bottleneck,
>that is, you
Hi,
I have been testing with newer compiler (clang-7)
and result is a bit different at least with clang-7.
Compiling PG 12.1 (even without patch) with clang-7
results in __isinf() no longer being a bottleneck,
that is, you don't see it in profiler at all.
So, there is no issue for people who use
Hi,
On 2020-02-06 11:03:51 -0500, Tom Lane wrote:
> Andres seems to be of the opinion that the compiler should be willing
> to ignore the semantic requirements of the C standard in order
> to rearrange the code back into the cheaper order. That sounds like
> wishful thinking to me ... even if it
On Thu, Feb 6, 2020 at 11:04 AM Tom Lane wrote:
> So it appears to me that what commit 6bf0bc842 did in this area was
> not just wrong, but disastrously so. Before that, we had a macro that
> evaluated isinf(val) before it evaluated the inf_is_valid condition.
> Now we have check_float[48]_val
So it appears to me that what commit 6bf0bc842 did in this area was
not just wrong, but disastrously so. Before that, we had a macro that
evaluated isinf(val) before it evaluated the inf_is_valid condition.
Now we have check_float[48]_val which do it the other way around.
That would be okay if
Hi,
On Thu, Feb 6, 2020 at 2:55 PM Andres Freund wrote:
> On 2020-02-06 14:25:03 +0900, keisuke kuroda wrote:
> > That's because check_float8_val() (in PG 12) is a function
> > whose arguments must be evaluated before
> > it is called (it is inline, but that's irrelevant),
> > whereas
Hi,
On 2020-02-06 14:25:03 +0900, keisuke kuroda wrote:
> That's because check_float8_val() (in PG 12) is a function
> whose arguments must be evaluated before
> it is called (it is inline, but that's irrelevant),
> whereas CHECKFLOATVAL() (in PG11) is a macro
> whose arguments are only
36 matches
Mail list logo
