Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-12 Thread Ingo Schwarze
Hi Martijn, Martijn van Duren wrote on Sun, Jan 09, 2022 at 08:09:20AM +0100: > I fully agree with your reasoning and also prefer this one over the > previous two diff. > > OK martijn@ Thanks for checking; committed. Also thanks to cheloha@ for his work on this issue and to milllert@ for his fe

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-08 Thread Martijn van Duren
I fully agree with your reasoning and also prefer this one over the previous two diff. OK martijn@ On Sun, 2022-01-09 at 00:45 +0100, Ingo Schwarze wrote: > Hi, > > Martijn van Duren wrote on Sat, Jan 08, 2022 at 08:30:20AM +0100: > > > Why not go for the following diff? > > It has a comparable

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-08 Thread Ingo Schwarze
Oopsie... Ingo Schwarze wrote on Sun, Jan 09, 2022 at 12:45:27AM +0100: > # Demonstate broken error handling in the old code. > >$ ./obj/wrap /obin/rev >$ echo $? > 42 Stupid me, this is what i meant, of course: $ ./obj/wrap /oldbin/rev $ echo $?

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-08 Thread Ingo Schwarze
Hi, Martijn van Duren wrote on Sat, Jan 08, 2022 at 08:30:20AM +0100: > Why not go for the following diff? > It has a comparable speed increase, but without the added complexity > of a second inner loop. Actually, i like the idea of not duplicating the loop, in cases where that is possible witho

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-08 Thread Todd C . Miller
On Sat, 08 Jan 2022 08:30:20 +0100, Martijn van Duren wrote: > Why not go for the following diff? > It has a comparable speed increase, but without the added complexity of > a second inner loop. Personally, that is what I would have done as well, but I am OK with either direction. - todd

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-07 Thread Martijn van Duren
On Fri, 2022-01-07 at 15:00 -0600, Scott Cheloha wrote: > On Fri, Jan 07, 2022 at 01:43:24PM -0600, Scott Cheloha wrote: > > > > [...] > > > > Like this? > > > > [...] > > Updated: make the for-loop update expressions match. > Why not go for the following diff? It has a comparable speed increa

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-07 Thread Todd C . Miller
On Fri, 07 Jan 2022 15:00:52 -0600, Scott Cheloha wrote: > Updated: make the for-loop update expressions match. Looks good to me. OK millert@ - todd

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-07 Thread Scott Cheloha
On Fri, Jan 07, 2022 at 01:43:24PM -0600, Scott Cheloha wrote: > > [...] > > Like this? > > [...] Updated: make the for-loop update expressions match. Index: rev.c === RCS file: /cvs/src/usr.bin/rev/rev.c,v retrieving revision 1.13

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-07 Thread Scott Cheloha
On Fri, Jan 07, 2022 at 07:28:30PM +0100, Ingo Schwarze wrote: > > [...] > > Scott Cheloha wrote on Fri, Jan 07, 2022 at 09:45:41AM -0600: > > > In rev(1), we call MB_CUR_MAX for every byte in the input stream. > > This is extremely expensive. > > [...] > > So i agree with jour general idea to

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-07 Thread Ingo Schwarze
Hi Scott, thank you for spending quite some work on our small utility programs, and sorry for failing to follow up as often as i should. Scott Cheloha wrote on Fri, Jan 07, 2022 at 09:45:41AM -0600: > In rev(1), we call MB_CUR_MAX for every byte in the input stream. > This is extremely expensive

Re: rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-07 Thread Todd C . Miller
Why not just store the result of MB_CUR_MAX in a variable and use that? It's value is not going to change during execution of the program. This made a big difference in sort(1) once upon a time. - todd

rev(1): pull MB_CUR_MAX out of the hot loop

2022-01-07 Thread Scott Cheloha
In rev(1), we call MB_CUR_MAX for every byte in the input stream. This is extremely expensive. It is much cheaper to call it once per line and use a simpler loop (see the inlined patch below) if the current locale doesn't handle multibyte characters: # -current rev(1), $ for i in $(jot 10); do na