Re: [PATCH] Performance Improvement For Copy From Binary Files

On Tue, Jul 14, 2020 at 2:02 PM vignesh C <vignesh21@gmail.com> wrote:
> On Tue, Jul 14, 2020 at 7:26 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > In CopyLoadRawBuf(), we could also change the condition if
> > (cstate->raw_buf_index < cstate->raw_buf_len) to if (BUF_BYTES > 0),
> > which looks clearer.
> >
> > Also, if we are going to use the macro more generally, let's make it
> > look less localized.  For example, rename it to RAW_BUF_BYTES similar
> > to RAW_BUF_SIZE and place their definitions close by.  It also seems
> > like a good idea to make 'cstate' a parameter for clarity.
> >
> > Attached v6.
> >
>
> Thanks for making the changes.
>
> -       if (cstate->raw_buf_index < cstate->raw_buf_len)
> +       if (RAW_BUF_BYTES(cstate) > 0)
>         {
>                 /* Copy down the unprocessed data */
> -               nbytes = cstate->raw_buf_len - cstate->raw_buf_index;
> +               nbytes = RAW_BUF_BYTES(cstate);
>                 memmove(cstate->raw_buf, cstate->raw_buf +
> cstate->raw_buf_index,
>                                 nbytes);
>         }
>
> One small improvement could be to change it like below to reduce few
> more instructions:
> static bool
> CopyLoadRawBuf(CopyState cstate)
> {
> int nbytes = RAW_BUF_BYTES(cstate);
> int inbytes;
>
> /* Copy down the unprocessed data */
> if (nbytes > 0)
> memmove(cstate->raw_buf, cstate->raw_buf + cstate->raw_buf_index,
> nbytes);
>
> inbytes = CopyGetData(cstate, cstate->raw_buf + nbytes,
>   1, RAW_BUF_SIZE - nbytes);
> nbytes += inbytes;
> cstate->raw_buf[nbytes] = '\0';
> cstate->raw_buf_index = 0;
> cstate->raw_buf_len = nbytes;
> return (inbytes > 0);
> }

Sounds fine to me.  Although CopyLoadRawBuf() does not seem to a
candidate for rigorous code optimization as it does not get called
that often.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com