On Tue, 1 Aug 2023 at 01:48, Julien Rouhaud <rjuju...@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
join_search_one_level_tidyup.patch
Description: Binary data
