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 without a noticeable performance cost.
I admit i OK'ed the original UTF-8 diff almost six years ago,
but looking at the code again, i now dislike how it is testing
every byte twice for no apparent reason. Even worse, i regard
the lack of I/O error checking on write operations as a bug.
Note that it does make sense to proceed to the next file on *read*
errors, whereas a *write* error should better be fatal.
For those reason, and because i prefer versions of isu8cont()
that do not inspect the locale, i propose the following patch
instead.
As an aside, i tried using fwrite(3) instead of the manual
putchar(*u) loop, but that is almost exactly as slow as repeatedly
calling MB_CUR_MAX, probably due to either locking or orientation
setting or some aspect of iov handling or buffering or more than
one of those; i refrained from profiling it.
----- 8< ----- schnipp ----- >8 ----- 8< ----- schnapp ----- >8 -----
Index: rev.c
===================================================================
RCS file: /cvs/src/usr.bin/rev/rev.c,v
retrieving revision 1.13
diff -u -p -r1.13 rev.c
--- rev.c 10 Apr 2016 17:06:52 -0000 1.13
+++ rev.c 8 Jan 2022 23:19:46 -0000
@@ -46,13 +46,14 @@ void usage(void);
int
main(int argc, char *argv[])
{
- char *filename, *p = NULL, *t, *u;
+ char *filename, *p = NULL, *t, *te, *u;
FILE *fp;
ssize_t len;
size_t ps = 0;
- int ch, rval;
+ int ch, multibyte, rval;
setlocale(LC_CTYPE, "");
+ multibyte = MB_CUR_MAX > 1;
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
@@ -83,14 +84,16 @@ main(int argc, char *argv[])
if (p[len - 1] == '\n')
--len;
for (t = p + len - 1; t >= p; --t) {
- if (isu8cont(*t))
- continue;
- u = t;
- do {
- putchar(*u);
- } while (isu8cont(*(++u)));
+ te = t;
+ if (multibyte)
+ while (t > p && isu8cont(*t))
+ --t;
+ for (u = t; u <= te; ++u)
+ if (putchar(*u) == EOF)
+ err(1, "stdout");
}
- putchar('\n');
+ if (putchar('\n') == EOF)
+ err(1, "stdout");
}
if (ferror(fp)) {
warn("%s", filename);
@@ -104,7 +107,7 @@ main(int argc, char *argv[])
int
isu8cont(unsigned char c)
{
- return MB_CUR_MAX > 1 && (c & (0x80 | 0x40)) == 0x80;
+ return (c & (0x80 | 0x40)) == 0x80;
}
void
----- 8< ----- schnipp ----- >8 ----- 8< ----- schnapp ----- >8 -----
The rest of this mail contains test reports.
These test reports use two test programs:
----- 8< ----- schnipp ----- >8 ----- 8< ----- schnapp ----- >8 -----
/* makeutf8.c */
#include <err.h>
#include <errno.h>
#include <locale.h>
#include <stdio.h>
#include <wchar.h>
int
main(void)
{
FILE *in, *out;
char *line = NULL;
size_t linesize = 0;
unsigned int wc, wcl = 0;
if (setlocale(LC_CTYPE, "en_US.UTF-8") == NULL)
err(1, "setlocale");
if ((in = fopen("/usr/src/gnu/usr.bin/perl/lib/unicore/"
"UnicodeData.txt", "r")) == NULL)
err(1, "in");
if ((out = fopen("utf8.txt", "w")) == NULL)
err(1, "out");
while (getline(&line, &linesize, in) != -1) {
if (sscanf(line, "%x;", &wc) < 1)
err(1, "scanf: %s", line);
if (wc > wcl + 1)
if (fputwc(L'\n', out) == WEOF)
err(1, "write EOL");
if (fputwc(wc, out) == WEOF) {
if (errno == EILSEQ)
warn("write U+%04x", wc);
else
err(1, "write U+%04x", wc);
}
wcl = wc;
}
if (ferror(in))
errx(1, "read error");
if (fputwc(L'\n', out) == WEOF)
err(1, "write final EOL");
fclose(out);
fclose(in);
return 0;
}
----- 8< ----- schnipp ----- >8 ----- 8< ----- schnapp ----- >8 -----
/* wrap.c */
#include <unistd.h>
int
main(int argc, char *argv[])
{
close(STDOUT_FILENO);
execl(argv[1], argv[1], "/usr/share/dict/words", NULL);
return 42;
}
----- 8< ----- schnipp ----- >8 ----- 8< ----- schnapp ----- >8 -----
# Prepare for testing.
$ make makeutf8
cc -O2 -pipe -Werror-implicit-function-declaration -MD -MP \
-o makeutf8 /usr/src/usr.bin/rev/makeutf8.c
$ make wrap
cc -O2 -pipe -Werror-implicit-function-declaration -MD -MP \
-o wrap /usr/src/usr.bin/rev/wrap.c
$ make
cc -O2 -pipe -Werror-implicit-function-declaration -MD -MP \
-c /usr/src/usr.bin/rev/rev.c
cc -o rev rev.o
$ ./obj/makeutf8
makeutf8: write U+d800: Illegal byte sequence
makeutf8: write U+db7f: Illegal byte sequence
makeutf8: write U+db80: Illegal byte sequence
makeutf8: write U+dbff: Illegal byte sequence
makeutf8: write U+dc00: Illegal byte sequence
makeutf8: write U+dfff: Illegal byte sequence
$ wc utf8.txt
695 693 116982 utf8.txt
# Check that the current code works properly.
$ unset LC_ALL
$ export LC_CTYPE=C
$ /oldbin/rev /usr/share/dict/words > words_ocr.txt
$ /oldbin/rev words_ocr.txt > words_ocrr.txt
$ /oldbin/rev utf8.txt > utf8_ocr.txt
$ /oldbin/rev utf8_ocr.txt > utf8_ocrr.txt
$ export LC_CTYPE=en_US.UTF-8
$ /oldbin/rev /usr/share/dict/words > words_our.txt
$ /oldbin/rev words_our.txt > words_ourr.txt
$ /oldbin/rev utf8.txt > utf8_our.txt
$ /oldbin/rev utf8_our.txt > utf8_ourr.txt
$ diff -u /usr/share/dict/words words_ocrr.txt
$ diff -u /usr/share/dict/words words_ourr.txt
$ diff -u words_ocr.txt words_our.txt
$ diff -u utf8.txt utf8_ocrr.txt
$ diff -u utf8.txt utf8_ourr.txt
$ cmp utf8_ocr.txt utf8_our.txt
utf8_ocr.txt utf8_our.txt differ: char 12, line 2
# Check that the new code works properly.
$ export LC_CTYPE=C
$ ./obj/rev /usr/share/dict/words > words_ncr.txt
$ ./obj/rev utf8.txt > utf8_ncr.txt
$ export LC_CTYPE=en_US.UTF-8
$ ./obj/rev /usr/share/dict/words > words_nur.txt
$ ./obj/rev utf8.txt > utf8_nur.txt
$ diff -u words_ocr.txt words_ncr.txt
$ diff -u words_our.txt words_nur.txt
$ diff -u utf8_ocr.txt utf8_ncr.txt
$ diff -u utf8_our.txt utf8_nur.txt
$
# Demonstate broken error handling in the old code.
$ ./obj/wrap /obin/rev
$ echo $?
42
# Demonstrate working error handling in the new code.
$ ./obj/wrap ./obj/rev
rev: stdout: Bad file descriptor
$ echo $?
1
# Check performance.
$ export LC_CTYPE=C
$ for i in $(jot 10); do time /oldbin/rev words_ocr.txt > /dev/null; done
0m00.09s real 0m00.10s user 0m00.01s system
0m00.09s real 0m00.10s user 0m00.00s system
0m00.09s real 0m00.10s user 0m00.01s system
0m00.09s real 0m00.10s user 0m00.00s system
0m00.09s real 0m00.11s user 0m00.00s system
0m00.09s real 0m00.10s user 0m00.00s system
0m00.09s real 0m00.09s user 0m00.01s system
0m00.09s real 0m00.09s user 0m00.01s system
0m00.09s real 0m00.08s user 0m00.01s system
0m00.09s real 0m00.09s user 0m00.00s system
$ for i in $(jot 10); do time ./obj/rev words_ocr.txt > /dev/null; done
0m00.02s real 0m00.03s user 0m00.00s system
0m00.02s real 0m00.03s user 0m00.00s system
0m00.02s real 0m00.02s user 0m00.00s system
0m00.02s real 0m00.02s user 0m00.00s system
0m00.02s real 0m00.02s user 0m00.02s system
0m00.02s real 0m00.03s user 0m00.00s system
0m00.02s real 0m00.02s user 0m00.00s system
0m00.02s real 0m00.02s user 0m00.00s system
0m00.02s real 0m00.03s user 0m00.00s system
0m00.02s real 0m00.03s user 0m00.00s system
$ export LC_CTYPE=en_US.UTF-8
$ for i in $(jot 10); do time /oldbin/rev words_ocr.txt > /dev/null; done
0m00.09s real 0m00.10s user 0m00.00s system
0m00.10s real 0m00.12s user 0m00.00s system
0m00.09s real 0m00.10s user 0m00.00s system
0m00.09s real 0m00.10s user 0m00.00s system
0m00.09s real 0m00.09s user 0m00.01s system
0m00.09s real 0m00.10s user 0m00.00s system
0m00.09s real 0m00.10s user 0m00.00s system
0m00.09s real 0m00.08s user 0m00.01s system
0m00.09s real 0m00.09s user 0m00.00s system
0m00.09s real 0m00.09s user 0m00.00s system
$ for i in $(jot 10); do time ./obj/rev words_ocr.txt > /dev/null; done
0m00.03s real 0m00.02s user 0m00.01s system
0m00.03s real 0m00.02s user 0m00.00s system
0m00.03s real 0m00.03s user 0m00.00s system
0m00.03s real 0m00.03s user 0m00.00s system
0m00.03s real 0m00.03s user 0m00.00s system
0m00.03s real 0m00.03s user 0m00.00s system
0m00.03s real 0m00.02s user 0m00.01s system
0m00.03s real 0m00.03s user 0m00.00s system
0m00.03s real 0m00.02s user 0m00.01s system
0m00.03s real 0m00.03s user 0m00.00s system
# Regression tests.
$ doas make install
$ cd /usr/src/regress/usr.bin/rev/
$ make clean
rm -f a.out [Ee]rrs mklog *.core y.tab.h out.ascii.txt out.utf8.txt
$ make regress ; echo $?
0
