pg.ddx.io pgsql-general@postgresql.org mailing list archive
help / color / mirror / Atom feedMissed compiler optimization issue in function select_rtable_names_for_explain
10+ messages / 4 participants
[nested] [flat]
* Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 09:27 XChy <xxs_chy@outlook.com>
0 siblings, 2 replies; 10+ messages in thread
From: XChy @ 2024-05-22 09:27 UTC (permalink / raw)
To: pgsql-general
Hi everyone,
I'm a compiler developer working on detecting missed optimization in
real-world applications. Recently, we found that LLVM missed a dead
store elimination optimization in the PostgreSQL code
<https://github.com/postgres/postgres/blob/c37267162e889fe783786b9e28d1b65b82365a00/src/backend/utils...;
(https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/ruleutils.c#L3794)
in the master branch.
For the example below:
```
int dst[128];
memset(dst, 0, 128);
*unrelated = some_value;
dst[1] = 0;
dst[2] = 0;
```
LLVM cannot eliminate the useless stores after memset since the store to
"unrelated" clobbers. But if we put the stores to "dst" ahead of the
store to "unrelated", we could prevent confusing the compiler. See also
the compiler explorer link: https://godbolt.org/z/P9jnKod3v and the
issue of LLVM: https://github.com/llvm/llvm-project/issues/88632
To improve the codegen quality, I think it's also possible to modify the
source code, changing the order of initialization of the member, to get
better optimization. But I don't know whether this can be considered as
a bug, thus post the issue here.
If anyone could confirm this problem or post a patch for it, let me know
please. Thanks!
Best regards. Hongyu.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 09:51 Daniel Gustafsson <daniel@yesql.se>
parent: XChy <xxs_chy@outlook.com>
1 sibling, 1 reply; 10+ messages in thread
From: Daniel Gustafsson @ 2024-05-22 09:51 UTC (permalink / raw)
To: XChy <xxs_chy@outlook.com>; +Cc: pgsql-general
> On 22 May 2024, at 11:27, XChy <xxs_chy@outlook.com> wrote:
>
> Hi everyone,
> I'm a compiler developer working on detecting missed optimization in real-world applications. Recently, we found that LLVM missed a dead store elimination optimization in the PostgreSQL code (https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/ruleutils.c#L3794) in the master branch.
How is the memset in select_rtable_names_for_explain a dead-store? Even memset
calls could be optimized away from the EXPLAIN codepath I have a feeling it
would have to be many in a tight loop for it to be measurable even?
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 10:12 XChy <xxs_chy@outlook.com>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 10+ messages in thread
From: XChy @ 2024-05-22 10:12 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-general
> How is the memset in select_rtable_names_for_explain a dead-store?
> Even memset
> calls could be optimized away from the EXPLAIN codepath I have a feeling it
> would have to be many in a tight loop for it to be measurable even?
>
> --
> Daniel Gustafsson
For the first question, I don't mean that the memset is the dead store.
I mean that the stores with value "0" after the memset are dead:
```
dpns.subplans = NIL;
dpns.ctes = NIL;
dpns.appendrels = NULL;
```
since the memset has written zeroes to the object "dpns", and these
members are known to be zero.
For the second question, you are right, I don't really profile it or
measure the performance impact for it. I just think it's worthwhile to
improve codegen quality without affecting readability, as adopting
performance tips from some static analyzer.
Best regards, Hongyu.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 10:55 Daniel Gustafsson <daniel@yesql.se>
parent: XChy <xxs_chy@outlook.com>
0 siblings, 2 replies; 10+ messages in thread
From: Daniel Gustafsson @ 2024-05-22 10:55 UTC (permalink / raw)
To: XChy <xxs_chy@outlook.com>; +Cc: pgsql-general
> On 22 May 2024, at 12:12, XChy <xxs_chy@outlook.com> wrote:
>
>> How is the memset in select_rtable_names_for_explain a dead-store? Even memset calls could be optimized away from the EXPLAIN codepath I have a feeling it
>> would have to be many in a tight loop for it to be measurable even?
> For the first question, I don't mean that the memset is the dead store.
Gotcha
> I mean that the stores with value "0" after the memset are dead:
> ```
> dpns.subplans = NIL;
> dpns.ctes = NIL;
> dpns.appendrels = NULL;
> ```
> since the memset has written zeroes to the object "dpns", and these members are known to be zero.
They are known to be zero, but that's not entirely equivalent though is it?
NIL is defined as ((List *) NULL) and NULL is typically defined as ((void *)
0), so sizeof(0) would be the size of an int and sizeof(NULL) would be the size
of a void pointer.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 11:00 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: XChy <xxs_chy@outlook.com>
1 sibling, 2 replies; 10+ messages in thread
From: Alvaro Herrera @ 2024-05-22 11:00 UTC (permalink / raw)
To: XChy <xxs_chy@outlook.com>; +Cc: pgsql-general
On 2024-May-22, XChy wrote:
> Hi everyone,
>
> I'm a compiler developer working on detecting missed optimization in
> real-world applications. Recently, we found that LLVM missed a dead store
> elimination optimization in the PostgreSQL code <https://github.com/postgres/postgres/blob/c37267162e889fe783786b9e28d1b65b82365a00/src/backend/utils...; (https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/ruleutils.c#L3794)
> in the master branch.
I think if we want to improve how this code is seen by the compiler by
modifying it, we should just remove the NULL/NIL assignments. It's a
pretty arbitrary (and rather small) subset of fields being initialized,
fields which very obviously have been zeroed by memset already, so I
don't see any value in the explicit assignments.
The criteria may be different in cases where some struct has all of its
members initialized to something, because at least you can use that kind
of thing to find places you need to modify when adding new struct
members. Not so in select_rtable_names_for_explain.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"I can see support will not be a problem. 10 out of 10." (Simon Wittber)
(http://archives.postgresql.org/pgsql-general/2004-12/msg00159.php)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 13:41 XChy <xxs_chy@outlook.com>
parent: Daniel Gustafsson <daniel@yesql.se>
1 sibling, 0 replies; 10+ messages in thread
From: XChy @ 2024-05-22 13:41 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-general
在 2024/5/22 18:55, Daniel Gustafsson 写道:
>> I mean that the stores with value "0" after the memset are dead:
>> ```
>> dpns.subplans = NIL;
>> dpns.ctes = NIL;
>> dpns.appendrels = NULL;
>> ```
>> since the memset has written zeroes to the object "dpns", and these members are known to be zero.
> They are known to be zero, but that's not entirely equivalent though is it?
> NIL is defined as ((List *) NULL) and NULL is typically defined as ((void *)
> 0), so sizeof(0) would be the size of an int and sizeof(NULL) would be the size
> of a void pointer.
The type or size doesn't matter here. At IR or assembly level, they are
all zeroes.
My main point is that "dpns.xxx" are filled with zeroes by the memset
firstly, and overwriting them with zeroes in the following stores is
redundant. LLVM cannot remove the redundant overwrites due to the
initialization order. If we adjust the order of the initialization of
"dpns.xxx", the compiler can remove such stores.
Does my explanation make sense to you?
Best regards, Hongyu.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 16:53 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
1 sibling, 1 reply; 10+ messages in thread
From: Tom Lane @ 2024-05-22 16:53 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: XChy <xxs_chy@outlook.com>; pgsql-general
Daniel Gustafsson <daniel@yesql.se> writes:
> They are known to be zero, but that's not entirely equivalent though is it?
> NIL is defined as ((List *) NULL) and NULL is typically defined as ((void *)
> 0), so sizeof(0) would be the size of an int and sizeof(NULL) would be the size
> of a void pointer.
There are other places where we assume that a memset-to-zero will
produce null pointers, so I don't think that that objection has
a lot of force. My real answer is that this is our coding style
and we are not going to change it: our normal convention is to
initialize struct fields in declaration order, and that's what
we're doing here. If some particular version of some particular
compiler fails to make an entirely-negligible optimization in
consequence, that is not something we are going to care about.
Maybe we would care if the missed optimization were a serious
performance loss in a very hot code path. But this is neither.
I'd also say that this is pretty clearly a compiler bug.
If it'd normally optimize away a null-pointer-store following
a memset-to-zero, but does not in
memset(&dpns, 0, sizeof(dpns));
dpns.rtable = rtable;
dpns.subplans = NIL;
dpns.ctes = NIL;
dpns.appendrels = NULL;
that would seem to indicate that the optimizer doesn't
really understand that dpns.rtable is a distinct field
from the others. How is that our problem? (I kind of
wonder if what's actually blocking the optimization is
the casts inside the NIL macros. Still not our problem.)
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 18:12 Daniel Gustafsson <daniel@yesql.se>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 0 replies; 10+ messages in thread
From: Daniel Gustafsson @ 2024-05-22 18:12 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: XChy <xxs_chy@outlook.com>; pgsql-general
> On 22 May 2024, at 13:00, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I think if we want to improve how this code is seen by the compiler by
> modifying it, we should just remove the NULL/NIL assignments.
*If* the optimization is measurable, IMHO.
> It's a
> pretty arbitrary (and rather small) subset of fields being initialized,
> fields which very obviously have been zeroed by memset already, so I
> don't see any value in the explicit assignments.
My personal opinion is that there is value in immediately knowing the type of
the members from the inits, in this case I know without looking up the
definition that some of the members are list. I like that readability. That
being said, I for sure won't fight if there is concensus to remove.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 18:14 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Gustafsson @ 2024-05-22 18:14 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: XChy <xxs_chy@outlook.com>; pgsql-general
> On 22 May 2024, at 18:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> They are known to be zero, but that's not entirely equivalent though is it?
>> NIL is defined as ((List *) NULL) and NULL is typically defined as ((void *)
>> 0), so sizeof(0) would be the size of an int and sizeof(NULL) would be the size
>> of a void pointer.
>
> There are other places where we assume that a memset-to-zero will
> produce null pointers, so I don't think that that objection has
> a lot of force.
It wasn't really an objection, but (perhaps a badly worded) an attempt to
understand the proposal.
> My real answer is that this is our coding style
> and we are not going to change it: our normal convention is to
> initialize struct fields in declaration order, and that's what
> we're doing here. If some particular version of some particular
> compiler fails to make an entirely-negligible optimization in
> consequence, that is not something we are going to care about.
+1
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Missed compiler optimization issue in function select_rtable_names_for_explain
@ 2024-05-22 20:05 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 0 replies; 10+ messages in thread
From: Tom Lane @ 2024-05-22 20:05 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: XChy <xxs_chy@outlook.com>; pgsql-general
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I think if we want to improve how this code is seen by the compiler by
> modifying it, we should just remove the NULL/NIL assignments. It's a
> pretty arbitrary (and rather small) subset of fields being initialized,
> fields which very obviously have been zeroed by memset already, so I
> don't see any value in the explicit assignments.
I have to concede that it's unclear why we're filling just these
fields explicitly and not any others. We're at least consistent:
all the callers of select_rtable_names do it the same way.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-05-22 20:05 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-05-22 09:27 Missed compiler optimization issue in function select_rtable_names_for_explain XChy <xxs_chy@outlook.com>
2024-05-22 09:51 ` Daniel Gustafsson <daniel@yesql.se>
2024-05-22 10:12 ` XChy <xxs_chy@outlook.com>
2024-05-22 10:55 ` Daniel Gustafsson <daniel@yesql.se>
2024-05-22 13:41 ` XChy <xxs_chy@outlook.com>
2024-05-22 16:53 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-05-22 18:14 ` Daniel Gustafsson <daniel@yesql.se>
2024-05-22 11:00 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-05-22 18:12 ` Daniel Gustafsson <daniel@yesql.se>
2024-05-22 20:05 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox