Thread: Micro-optimizations to avoid some strlen calls.
Hi,
There are some places, where strlen can have an overhead.
This patch tries to fix this.
Pass check-world at linux ubuntu (20.04) 64 bits.
regards,
Ranier Vilela
Attachment
On Mon, Jul 19, 2021 at 07:48:55PM -0300, Ranier Vilela wrote: > There are some places, where strlen can have an overhead. > This patch tries to fix this. > > Pass check-world at linux ubuntu (20.04) 64 bits. Why does it matter? No code paths you are changing here are performance-critical, meaning that such calls won't really show up high in profiles. I don't think there is anything to change here. -- Michael
Attachment
On Tue, Jul 20, 2021 at 5:28 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Jul 19, 2021 at 07:48:55PM -0300, Ranier Vilela wrote:
> There are some places, where strlen can have an overhead.
> This patch tries to fix this.
>
> Pass check-world at linux ubuntu (20.04) 64 bits.
Why does it matter? No code paths you are changing here are
performance-critical, meaning that such calls won't really show up
high in profiles.
I don't think there is anything to change here.
Agreed. To borrow from a nearby email of a similar nature (PGConn information retrieval IIRC) - it is not generally a benefit to avoid function call access to data multiple times in a block by substituting in a saved local variable. The function call tends to be more readable then having yet one more unimportant name to keep in short-term memory. As much code already conforms to that the status quo is a preferred state unless there is a demonstrable performance gain to be had. The readability, and lack of churn, is otherwise more important.
David J.
On Tue, 20 Jul 2021 at 10:49, Ranier Vilela <ranier.vf@gmail.com> wrote:
> There are some places, where strlen can have an overhead.
> This patch tries to fix this.
I'm with Michael and David on this.
I don't really feel like doing;
- snprintf(buffer, sizeof(buffer), "E%s%s\n",
+ buflen = snprintf(buffer, sizeof(buffer), "E%s%s\n",
_("could not fork new process for connection: "),
is a good idea. I'm unsure if you're either not aware of the value
that snprintf() returns or just happen to think an overflow is
unlikely enough because you're convinced that 1000 chars are always
enough to fit this translatable string. I'd say if we were 100%
certain of that then it might as well become sprintf() instead.
However, I imagine you'll struggle to get people to side with you that
taking this overflow risk would be worthwhile given your lack of any
evidence that anything actually has become meaningfully faster as a
result of any of these changes.
David
Em qua., 21 de jul. de 2021 às 07:44, David Rowley <dgrowleyml@gmail.com> escreveu:
On Tue, 20 Jul 2021 at 10:49, Ranier Vilela <ranier.vf@gmail.com> wrote:
> There are some places, where strlen can have an overhead.
> This patch tries to fix this.
I'm with Michael and David on this.
I don't really feel like doing;
- snprintf(buffer, sizeof(buffer), "E%s%s\n",
+ buflen = snprintf(buffer, sizeof(buffer), "E%s%s\n",
_("could not fork new process for connection: "),
is a good idea. I'm unsure if you're either not aware of the value
that snprintf() returns or just happen to think an overflow is
unlikely enough because you're convinced that 1000 chars are always
enough to fit this translatable string. I'd say if we were 100%
certain of that then it might as well become sprintf() instead.
However, I imagine you'll struggle to get people to side with you that
taking this overflow risk would be worthwhile given your lack of any
evidence that anything actually has become meaningfully faster as a
result of any of these changes.
I got your point.
Really getting only the result of snprintf is a bad idea.
In this case, the right way would be:
snprintf(buffer, sizeof(buffer), "E%s%s\n",
_("could not fork new process for connection: "),
_("could not fork new process for connection: "),
buflen = strlen(buffer);
Thus doesn't have to recount buffer over, if rc fails.
Thanks for the tip about snprintf, even though it's not the intention.
This is what I call a bad interface.
regards,
Ranier Vilela
Em qua., 21 de jul. de 2021 às 09:28, Ranier Vilela <ranier.vf@gmail.com> escreveu:
Em qua., 21 de jul. de 2021 às 07:44, David Rowley <dgrowleyml@gmail.com> escreveu:On Tue, 20 Jul 2021 at 10:49, Ranier Vilela <ranier.vf@gmail.com> wrote:
> There are some places, where strlen can have an overhead.
> This patch tries to fix this.
I'm with Michael and David on this.
I don't really feel like doing;
- snprintf(buffer, sizeof(buffer), "E%s%s\n",
+ buflen = snprintf(buffer, sizeof(buffer), "E%s%s\n",
_("could not fork new process for connection: "),
is a good idea. I'm unsure if you're either not aware of the value
that snprintf() returns or just happen to think an overflow is
unlikely enough because you're convinced that 1000 chars are always
enough to fit this translatable string. I'd say if we were 100%
certain of that then it might as well become sprintf() instead.
However, I imagine you'll struggle to get people to side with you that
taking this overflow risk would be worthwhile given your lack of any
evidence that anything actually has become meaningfully faster as a
result of any of these changes.I got your point.Really getting only the result of snprintf is a bad idea.In this case, the right way would be:snprintf(buffer, sizeof(buffer), "E%s%s\n",
_("could not fork new process for connection: "),buflen = strlen(buffer);Thus doesn't have to recount buffer over, if rc fails.Thanks for the tip about snprintf, even though it's not the intention.This is what I call a bad interface.
Here the v1 version, with fix to snprintf trap.
regards,
Ranier Vilela