On Tue, 1 Aug 2023 at 01:48, Julien Rouhaud <rjuju123@gmail.com> 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