Re: [PATCH 047/112] string: reduce strjoin runtime, drop trailing separator
On Mon, Jan 08, 2024 at 08:18:46AM +0100, Ahmad Fatoum wrote:
> On 08.01.24 08:11, Sascha Hauer wrote:
> > On Wed, Jan 03, 2024 at 07:12:07PM +0100, Ahmad Fatoum wrote:
> >> The implementation of strjoin is a bit suboptimal. The destination
> >> string is traversed from the beginning due to strcat and we have a
> >> left-over separator at the end, while it should only be in-between.
> >>
> >> Fix this.
> >>
> >> Signed-off-by: Ahmad Fatoum
> >> ---
> >> Originally posted at:
> >> https://lore.barebox.org/barebox/20221027073334.gs6...@pengutronix.de/
> >
> > Once again I ended up reviewing a suboptimal version of strjoin() first
> > just to find my potential comments addressed in the next patch. So my
> > comment to the last version still stands: Please implement a good
> > version of strjoin() first and then switch over to use it.
>
> The first patch just moves code around. I find it completely valid to move
> code before doing changes to it...
I find this valid as well, but you have not just moved it around, you
have introduced a new library function in the same step. As such it
should be in good shape when introducing it.
Sascha
>
> >
> >>
> >> Changes:
> >> - remove if statemnt in loop (Sascha)
> >> Signed-off-by: Ahmad Fatoum
> >> ---
> >> lib/string.c | 15 ++-
> >> 1 file changed, 10 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/lib/string.c b/lib/string.c
> >> index d8e5edd40648..695e50bc8fc1 100644
> >> --- a/lib/string.c
> >> +++ b/lib/string.c
> >> @@ -1005,7 +1005,7 @@ char *strjoin(const char *separator, char **arr,
> >> size_t arrlen)
> >> {
> >>size_t separatorlen;
> >>int len = 1; /* '\0' */
> >> - char *buf;
> >> + char *buf, *p;
> >>int i;
> >>
> >>separatorlen = strlen(separator);
> >> @@ -1013,13 +1013,18 @@ char *strjoin(const char *separator, char **arr,
> >> size_t arrlen)
> >>for (i = 0; i < arrlen; i++)
> >>len += strlen(arr[i]) + separatorlen;
> >
> > Not that it matters much for memory usage, but for consistency you could
> > drop the final separatorlen just like you did for copying the strings
> > below.
> >
> > Sascha
> >
> >>
> >> - buf = xzalloc(len);
> >> + if (!arrlen)
> >> + return xzalloc(1);
> >>
> >> - for (i = 0; i < arrlen; i++) {
> >> - strcat(buf, arr[i]);
> >> - strcat(buf, separator);
> >> + p = buf = xmalloc(len);
> >> +
> >> + for (i = 0; i < arrlen - 1; i++) {
> >> + p = stpcpy(p, arr[i]);
> >> + p = mempcpy(p, separator, separatorlen);
> >>}
> >>
> >> + stpcpy(p, arr[i]);
> >> +
> >>return buf;
> >> }
> >> EXPORT_SYMBOL(strjoin);
> >> --
> >> 2.39.2
> >>
> >>
> >>
> >
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 047/112] string: reduce strjoin runtime, drop trailing separator
On 08.01.24 08:11, Sascha Hauer wrote:
> On Wed, Jan 03, 2024 at 07:12:07PM +0100, Ahmad Fatoum wrote:
>> The implementation of strjoin is a bit suboptimal. The destination
>> string is traversed from the beginning due to strcat and we have a
>> left-over separator at the end, while it should only be in-between.
>>
>> Fix this.
>>
>> Signed-off-by: Ahmad Fatoum
>> ---
>> Originally posted at:
>> https://lore.barebox.org/barebox/20221027073334.gs6...@pengutronix.de/
>
> Once again I ended up reviewing a suboptimal version of strjoin() first
> just to find my potential comments addressed in the next patch. So my
> comment to the last version still stands: Please implement a good
> version of strjoin() first and then switch over to use it.
The first patch just moves code around. I find it completely valid to move
code before doing changes to it...
>
>>
>> Changes:
>> - remove if statemnt in loop (Sascha)
>> Signed-off-by: Ahmad Fatoum
>> ---
>> lib/string.c | 15 ++-
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/string.c b/lib/string.c
>> index d8e5edd40648..695e50bc8fc1 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -1005,7 +1005,7 @@ char *strjoin(const char *separator, char **arr,
>> size_t arrlen)
>> {
>> size_t separatorlen;
>> int len = 1; /* '\0' */
>> -char *buf;
>> +char *buf, *p;
>> int i;
>>
>> separatorlen = strlen(separator);
>> @@ -1013,13 +1013,18 @@ char *strjoin(const char *separator, char **arr,
>> size_t arrlen)
>> for (i = 0; i < arrlen; i++)
>> len += strlen(arr[i]) + separatorlen;
>
> Not that it matters much for memory usage, but for consistency you could
> drop the final separatorlen just like you did for copying the strings
> below.
>
> Sascha
>
>>
>> -buf = xzalloc(len);
>> +if (!arrlen)
>> +return xzalloc(1);
>>
>> -for (i = 0; i < arrlen; i++) {
>> -strcat(buf, arr[i]);
>> -strcat(buf, separator);
>> +p = buf = xmalloc(len);
>> +
>> +for (i = 0; i < arrlen - 1; i++) {
>> +p = stpcpy(p, arr[i]);
>> +p = mempcpy(p, separator, separatorlen);
>> }
>>
>> +stpcpy(p, arr[i]);
>> +
>> return buf;
>> }
>> EXPORT_SYMBOL(strjoin);
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 047/112] string: reduce strjoin runtime, drop trailing separator
On Wed, Jan 03, 2024 at 07:12:07PM +0100, Ahmad Fatoum wrote:
> The implementation of strjoin is a bit suboptimal. The destination
> string is traversed from the beginning due to strcat and we have a
> left-over separator at the end, while it should only be in-between.
>
> Fix this.
>
> Signed-off-by: Ahmad Fatoum
> ---
> Originally posted at:
> https://lore.barebox.org/barebox/20221027073334.gs6...@pengutronix.de/
Once again I ended up reviewing a suboptimal version of strjoin() first
just to find my potential comments addressed in the next patch. So my
comment to the last version still stands: Please implement a good
version of strjoin() first and then switch over to use it.
>
> Changes:
> - remove if statemnt in loop (Sascha)
> Signed-off-by: Ahmad Fatoum
> ---
> lib/string.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/lib/string.c b/lib/string.c
> index d8e5edd40648..695e50bc8fc1 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -1005,7 +1005,7 @@ char *strjoin(const char *separator, char **arr, size_t
> arrlen)
> {
> size_t separatorlen;
> int len = 1; /* '\0' */
> - char *buf;
> + char *buf, *p;
> int i;
>
> separatorlen = strlen(separator);
> @@ -1013,13 +1013,18 @@ char *strjoin(const char *separator, char **arr,
> size_t arrlen)
> for (i = 0; i < arrlen; i++)
> len += strlen(arr[i]) + separatorlen;
Not that it matters much for memory usage, but for consistency you could
drop the final separatorlen just like you did for copying the strings
below.
Sascha
>
> - buf = xzalloc(len);
> + if (!arrlen)
> + return xzalloc(1);
>
> - for (i = 0; i < arrlen; i++) {
> - strcat(buf, arr[i]);
> - strcat(buf, separator);
> + p = buf = xmalloc(len);
> +
> + for (i = 0; i < arrlen - 1; i++) {
> + p = stpcpy(p, arr[i]);
> + p = mempcpy(p, separator, separatorlen);
> }
>
> + stpcpy(p, arr[i]);
> +
> return buf;
> }
> EXPORT_SYMBOL(strjoin);
> --
> 2.39.2
>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
