Subject: bug#24159: [PATCH] dfa: minor fix for whether dfa is fast or not

From: Jim Meyering <[email-masked]>
Date: Sat, 6 Aug 2016 09:29:12 -0700

On Fri, Aug 5, 2016 at 10:05 PM, Jim Meyering <[email-masked]> wrote:
> On Fri, Aug 5, 2016 at 10:02 PM, Jim Meyering <[email-masked]> wrote:
>> On Fri, Aug 5, 2016 at 7:32 PM, Norihiro Tanaka <[email-masked]> wrote:
>>>
>>> On Fri, 5 Aug 2016 13:29:43 -0700
>>> Jim Meyering <[email-masked]> wrote:
>>>
>>>> On Fri, Aug 5, 2016 at 4:30 AM, Norihiro Tanaka <[email-masked]> wrote:
>>>> > dfaoptimize() is not set fast flag even if it is success, but it is wrong.
>>>> > If success, dfa matcher uses algorithm for single byte, and it is so fast.
>>>> >
>>>> > I think this bug does not affect for grep, but it will affect with the
>>>> > patch that I just sent to gawk.
>>>>
>>>> Thank you for the patch.
>>>> I was going to push it with the attached slightly updated log message.
>>>> Note however that grep does use that -> fast member via dfasearch.c's
>>>> use of the dfaisfast function.
>>>> But then I realized I should at least verify with "make check", and
>>>> found that this makes grep's dfa-match test fail.
>>>> Thus, I will not be pushing it as-is.
>>>
>>> Thanks for review and adjustment.  I re-ran all tests including dfa-match,
>>> and they were passwd again in my machine.  Next, I will re-run them on
>>> Fedora24, as my machine is RHEL 6.8 and GCC 4.4.7 which is too old.
>>>
>>> However, I do not know why dfa-match test fails on your machine.
>>> dfa-match test does not use grep.  It directly calls dfa functions through
>>> dfa-match-aux executable in order to test codes of dfa which grep does
>>> not use.  dfa-match-aux does not referer to the ->fast member.
>>
>> I have examined the logs, which suggest it was a false positive in a
>> parallelized "make check" run, due to that test's 3-second timeout. I
>> have tried repeatedly to reproduce that failure, so far without
>> success, but in coreutils development, with parallelized tests, we
>> fixed many hard-to-reproduce tests with small timeout limits like this
>> -- most of them now use 10 seconds as the limit, so I will change this
>> one, too (and several others) with the attached patch.
>>
>> I have pushed your patch.
>
> While trying to reproduce that, I ran some tests on OS X and saw
> frequent failure of one of the tests, so wrote the attached to work
> around what I assume is an aggressive write-to-/dev/null optimization:

Dug deeper and saw it was due to grep's own new stdout-vs-/dev/null
optimization.
I've updated the comment and pushed this:
From a2a3df953a91f898bd94f05a26088ccdc351fb3a Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Fri, 5 Aug 2016 21:38:30 -0700
Subject: [PATCH] tests/backref-multibyte-slow:: avoid false positive

* tests/backref-multibyte-slow: When redirecting the "fast" LC_ALL=C
run's output to /dev/null, we got an artificially low timing (of 0),
due to grep's own stdout-vs-/dev/null optimization.  With an initial
timing of 0 on that first run, the derived timeout for the UTF-8 run
(which redirects to a file) would be a mere 1 second.  The fix: also
redirect that first run's output to a file, not to /dev/null.
---
 tests/backref-multibyte-slow | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/backref-multibyte-slow b/tests/backref-multibyte-slow
index d447a4a..05bb62e 100755
--- a/tests/backref-multibyte-slow
+++ b/tests/backref-multibyte-slow
@@ -14,7 +14,7 @@ $AWK 'BEGIN {for (i=0; i<13000; i++) print "aba"}' /dev/null > in || fail=1
 # when running in en_US.UTF-8.  Round up to whole seconds, since timeout
 # can't deal with fractional seconds.
 max_seconds=$(LC_ALL=C perl -le 'use Time::HiRes qw(time); my $s = time();
-              system q,grep -E '\''^([a-z]).\1$'\'' in > /dev/null,;
+              system q,grep -E '\''^([a-z]).\1$'\'' in > junk,;
               my $elapsed = time() - $s; print int (1 + 10 * $elapsed)') \
   || { max_seconds=5;
        warn_ "$ME_: warning: no perl? using default of 5s timeout"; }
-- 
2.8.0-rc2