public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Julien Rouhaud <[email protected]>
Cc: 謝東霖 <[email protected]>
Cc: [email protected]
Subject: Re: Improve join_search_one_level readibilty (one line change)
Date: Fri, 4 Aug 2023 14:36:15 +1200
Message-ID: <CAApHDvrDPvj51Te76fOV+Yu6MvYKzFVmx4hZ0wZ0TmDqZ+R3+Q@mail.gmail.com> (raw)
In-Reply-To: <20230731134837.dpugorvuhtv27b3t@jrouhaud>
References: <CANWNU8x9P9aCXGF=aT-A_8mLTAT0LkcZ_ySYrGbcuHzMQw2-1g@mail.gmail.com>
<CANWNU8xm07jYUHxGh3XNHtcY37z+56-6bDD4piPt6=KidiHshQ@mail.gmail.com>
<CAOBaU_YTCo6F2kkhJy8jWmw--iDYHTEWeENunyu0Bc27VfO_KQ@mail.gmail.com>
<CANWNU8yxZ-SwGJvbbPuPmz2dUmn27q5CL885MWaD16+y-QuGEw@mail.gmail.com>
<20230731134837.dpugorvuhtv27b3t@jrouhaud>
On Tue, 1 Aug 2023 at 01:48, Julien Rouhaud <[email protected]> wrote:
> Apart from that +1 from me for the patch, I think it helps focusing the
> attention on what actually matters here.
I think it's worth doing something to improve this code. However, I
think we should go a bit further than what the proposed patch does.
In 1cff1b95a, Tom changed the signature of make_rels_by_clause_joins
to pass the List that the given ListCell belongs to. This was done
because lnext(), as of that commit, requires the owning List to be
passed to the function, where previously when Lists were linked lists,
that wasn't required.
The whole lnext() stuff all feels a bit old now that Lists are arrays.
I think we'd be better adjusting the code to pass the List index where
we start from rather than the ListCell to start from. That way we can
use for_each_from() to iterate rather than for_each_cell(). What's
there today feels a bit crufty and there's some element of danger that
the given ListCell does not even belong to the given List.
Doing this seems to shrink down the assembly a bit:
$ wc -l joinrels*
3344 joinrels_tidyup.s
3363 joinrels_unpatched.s
I also see a cmovne in joinrels_tidyup.s, which means that there are
fewer jumps which makes things a little better for the branch
predictor as there's fewer jumps. I doubt this is going to be
performance critical, but it's a nice extra bonus to go along with the
cleanup.
old:
cmpl $2, 24(%rsp)
je .L616
new:
cmpl $2, 16(%rsp)
cmovne %edx, %eax
David
Attachments:
[application/octet-stream] join_search_one_level_tidyup.patch (2.8K, ../CAApHDvrDPvj51Te76fOV+Yu6MvYKzFVmx4hZ0wZ0TmDqZ+R3+Q@mail.gmail.com/2-join_search_one_level_tidyup.patch)
download | inline diff:
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 015a0b3cbe..235c26d200 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -25,8 +25,8 @@
static void make_rels_by_clause_joins(PlannerInfo *root,
RelOptInfo *old_rel,
- List *other_rels_list,
- ListCell *other_rels);
+ List *other_rels,
+ int first_rel_idx);
static void make_rels_by_clauseless_joins(PlannerInfo *root,
RelOptInfo *old_rel,
List *other_rels);
@@ -93,6 +93,8 @@ join_search_one_level(PlannerInfo *root, int level)
if (old_rel->joininfo != NIL || old_rel->has_eclass_joins ||
has_join_restriction(root, old_rel))
{
+ int first_rel;
+
/*
* There are join clauses or join order restrictions relevant to
* this rel, so consider joins between this rel and (only) those
@@ -106,24 +108,12 @@ join_search_one_level(PlannerInfo *root, int level)
* to each initial rel they don't already include but have a join
* clause or restriction with.
*/
- List *other_rels_list;
- ListCell *other_rels;
-
if (level == 2) /* consider remaining initial rels */
- {
- other_rels_list = joinrels[level - 1];
- other_rels = lnext(other_rels_list, r);
- }
- else /* consider all initial rels */
- {
- other_rels_list = joinrels[1];
- other_rels = list_head(other_rels_list);
- }
+ first_rel = foreach_current_index(r) + 1;
+ else
+ first_rel = 0;
- make_rels_by_clause_joins(root,
- old_rel,
- other_rels_list,
- other_rels);
+ make_rels_by_clause_joins(root, old_rel, joinrels[1], first_rel);
}
else
{
@@ -286,9 +276,8 @@ join_search_one_level(PlannerInfo *root, int level)
* automatically ensures that each new joinrel is only added to the list once.
*
* 'old_rel' is the relation entry for the relation to be joined
- * 'other_rels_list': a list containing the other
- * rels to be considered for joining
- * 'other_rels': the first cell to be considered
+ * 'other_rels': a list containing the other rels to be considered for joining
+ * 'first_rel_idx': the first rel to be considered in 'other_rels'
*
* Currently, this is only used with initial rels in other_rels, but it
* will work for joining to joinrels too.
@@ -296,12 +285,12 @@ join_search_one_level(PlannerInfo *root, int level)
static void
make_rels_by_clause_joins(PlannerInfo *root,
RelOptInfo *old_rel,
- List *other_rels_list,
- ListCell *other_rels)
+ List *other_rels,
+ int first_rel_idx)
{
ListCell *l;
- for_each_cell(l, other_rels_list, other_rels)
+ for_each_from(l, other_rels, first_rel_idx)
{
RelOptInfo *other_rel = (RelOptInfo *) lfirst(l);
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Improve join_search_one_level readibilty (one line change)
In-Reply-To: <CAApHDvrDPvj51Te76fOV+Yu6MvYKzFVmx4hZ0wZ0TmDqZ+R3+Q@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox