Don't use bms_membership in places where it's not needed
While working on the patch in [1]/messages/by-id/CAApHDvqHCNKJi9CrQZG-reQDXTfRWnT5rhzNtDQhnrBzAAusfA@mail.gmail.com, I noticed that ever since
00b41463c, it's now suboptimal to do the following:
switch (bms_membership(relids))
{
case BMS_EMPTY_SET:
/* handle empty set */
break;
case BMS_SINGLETON:
/* call bms_singleton_member() and handle singleton set */
break;
case BMS_MULTIPLE:
/* handle multi-member set */
break;
}
The following is cheaper as we don't need to call bms_membership() and
bms_singleton_member() for singleton sets. It also saves function call
overhead for empty sets.
if (relids == NULL)
/* handle empty set */
else
{
int relid;
if (bms_get_singleton(relids, &relid))
/* handle singleton set */
else
/* handle multi-member set */
}
In the attached, I've adjusted the code to use the latter of the two
above methods in 3 places. In examine_variable() this reduces the
complexity of the logic quite a bit and saves calling bms_is_member()
in addition to bms_singleton_member().
I'm trying to reduce the footprint of what's being worked on in [1]/messages/by-id/CAApHDvqHCNKJi9CrQZG-reQDXTfRWnT5rhzNtDQhnrBzAAusfA@mail.gmail.com
and I highlighted this as something that'll help with that.
Any objections to me pushing the attached?
David
[1]: /messages/by-id/CAApHDvqHCNKJi9CrQZG-reQDXTfRWnT5rhzNtDQhnrBzAAusfA@mail.gmail.com
Attachments:
use_bms_membership_less_v1.patchtext/plain; charset=US-ASCII; name=use_bms_membership_less_v1.patchDownload+38-34
On Fri, Nov 24, 2023 at 12:06 PM David Rowley <dgrowleyml@gmail.com> wrote:
In the attached, I've adjusted the code to use the latter of the two
above methods in 3 places. In examine_variable() this reduces the
complexity of the logic quite a bit and saves calling bms_is_member()
in addition to bms_singleton_member().
+1 to the idea.
I think you have a typo in distribute_restrictinfo_to_rels. We should
remove the call of bms_singleton_member and use relid instead.
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2644,7 +2644,7 @@ distribute_restrictinfo_to_rels(PlannerInfo *root,
* There is only one relation participating in the clause, so it
* is a restriction clause for that relation.
*/
- rel = find_base_rel(root, bms_singleton_member(relids));
+ rel = find_base_rel(root, relid);Thanks
Richard
On Fri, 24 Nov 2023 at 19:54, Richard Guo <guofenglinux@gmail.com> wrote:
+1 to the idea.
I think you have a typo in distribute_restrictinfo_to_rels. We should
remove the call of bms_singleton_member and use relid instead.
Thanks for reviewing. I've now pushed this.
David
Hi,
On 2023-11-24 17:06:25 +1300, David Rowley wrote:
While working on the patch in [1], I noticed that ever since
00b41463c, it's now suboptimal to do the following:switch (bms_membership(relids))
{
case BMS_EMPTY_SET:
/* handle empty set */
break;
case BMS_SINGLETON:
/* call bms_singleton_member() and handle singleton set */
break;
case BMS_MULTIPLE:
/* handle multi-member set */
break;
}The following is cheaper as we don't need to call bms_membership() and
bms_singleton_member() for singleton sets. It also saves function call
overhead for empty sets.if (relids == NULL)
/* handle empty set */
else
{
int relid;if (bms_get_singleton(relids, &relid))
/* handle singleton set */
else
/* handle multi-member set */
}
Hm, does this ever matter from a performance POV? The current code does look
simpler to read to me. If the overhead is relevant, I'd instead just move the
code into a static inline?
Greetings,
Andres Freund
On Tue, 28 Nov 2023 at 11:21, Andres Freund <andres@anarazel.de> wrote:
Hm, does this ever matter from a performance POV? The current code does look
simpler to read to me. If the overhead is relevant, I'd instead just move the
code into a static inline?
I didn't particularly find the code in examine_variable() easy to
read. I think what's there now is quite a bit better than what was
there.
bms_get_singleton_member() was added in d25367ec4 for this purpose, so
it seems kinda weird not to use it.
David