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

On Tue, Jul 14, 2020 at 2:02 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> On Tue, Jul 14, 2020 at 7:26 AM Amit Langote <amitlangote09(at)gmail(dot)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