Re: Some ExecSeqScan optimizations

Hi Andres,

On Thu, Jul 10, 2025 at 8:34 AM Andres Freund <andres@anarazel.de> wrote:
> On 2025-01-22 10:07:51 +0900, Amit Langote wrote:
> > On Fri, Jan 17, 2025 at 2:05 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > Here's v5 with a few commit message tweaks.
> > >
> > > Barring objections, I would like to push this early next week.
> >
> > Pushed yesterday.  Thank you all.
>
> While looking at a profile I recently noticed that ExecSeqScanWithQual() had a
> runtime branch to test whether qual is NULL, which seemed a bit silly. I think
> we should use pg_assume(), which I just added to avoid a compiler warning, to
> improve the code generation here.

+1. I think this might be what David was getting at in his first
message in this thread.

> The performance gain unsurprisingly isn't significant (but seems repeatably
> measureable), but it does cut out a fair bit of unnecessary code.
>
> andres@awork3:/srv/dev/build/postgres/m-dev-optimize$ size executor_nodeSeqscan.c.*o
>    text    data     bss     dec     hex filename
>    3330       0       0    3330     d02 executor_nodeSeqscan.c.assume.o
>    3834       0       0    3834     efa executor_nodeSeqscan.c.o
>
> A 13% reduction in actual code size isn't bad for such a small change, imo.

Yeah, that seems worthwhile. I had been a bit concerned about code
size growth from having four variant functions with at least some
duplication, so this is a nice offset.

Thanks for the patch.

+    /*
+     * Use pg_assume() for != NULL tests to make the compiler realize no
+     * runtime check for the field is needed in ExecScanExtended().
+     */

I propose changing "to make the compiler realize no runtime check" to
"so the compiler can optimize away the runtime check", assuming that
is what it means.

Also, I assume you intentionally avoided repeating the comment in all
the variant functions.

> I have a separate question as well - do we need to call ResetExprContext() if
> we neither qual, projection nor epq?  I see a small gain by avoiding that.

You're referring to this block, I assume:

    /*
     * If we have neither a qual to check nor a projection to do, just skip
     * all the overhead and return the raw scan tuple.
     */
    if (!qual && !projInfo)
    {
        ResetExprContext(econtext);
        return ExecScanFetch(node, epqstate, accessMtd, recheckMtd);
    }

Yeah, I think it's fine to remove ResetExprContext() here. When I
looked at it before, I left it in because I was unsure whether
accessMtd() might leak memory into the per-tuple context. But on
second thought, that seems unlikely?  Would you like me to do it or do
you have a patch in your tree already?

--
Thanks, Amit Langote