GNU bug report logs - #24159
[PATCH] dfa: minor fix for whether dfa is fast or not

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24159 in the body.
You can then email your comments to 24159 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: <bug-grep <at> gnu.org>
Subject: [PATCH] dfa: minor fix for whether dfa is fast or not
Date: Fri, 05 Aug 2016 20:30:49 +0900

[Message part 1 (text/plain, inline)]

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.

[0001-dfa-minor-fix-for-whether-dfa-is-fast-or-not.patch (application/octet-stream, attachment)]

Message #8 received at 24159 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 24159 <at> debbugs.gnu.org
Subject: Re: bug#24159: [PATCH] dfa: minor fix for whether dfa is fast or not
Date: Fri, 5 Aug 2016 13:29:43 -0700

[Message part 1 (text/plain, inline)]

On Fri, Aug 5, 2016 at 4:30 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> 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.

[0001-dfa-minor-fix-for-whether-dfa-is-fast.diff (text/plain, attachment)]

Message #11 received at 24159 <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: Jim Meyering <jim <at> meyering.net>
Cc: 24159 <at> debbugs.gnu.org
Subject: Re: bug#24159: [PATCH] dfa: minor fix for whether dfa is fast or not
Date: Sat, 06 Aug 2016 11:32:46 +0900

On Fri, 5 Aug 2016 13:29:43 -0700
Jim Meyering <jim <at> meyering.net> wrote:

> On Fri, Aug 5, 2016 at 4:30 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> 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.

Message #16 received at 24159-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 24159-done <at> debbugs.gnu.org
Subject: Re: bug#24159: [PATCH] dfa: minor fix for whether dfa is fast or not
Date: Fri, 5 Aug 2016 22:02:31 -0700

[Message part 1 (text/plain, inline)]

On Fri, Aug 5, 2016 at 7:32 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
>
> On Fri, 5 Aug 2016 13:29:43 -0700
> Jim Meyering <jim <at> meyering.net> wrote:
>
>> On Fri, Aug 5, 2016 at 4:30 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> 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.

[0001-tests-standardize-on-10-second-timeouts-to-avoid-rar.diff (text/plain, attachment)]

Message #19 received at 24159-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 24159-done <at> debbugs.gnu.org
Subject: Re: bug#24159: [PATCH] dfa: minor fix for whether dfa is fast or not
Date: Fri, 5 Aug 2016 22:05:38 -0700

[Message part 1 (text/plain, inline)]

On Fri, Aug 5, 2016 at 10:02 PM, Jim Meyering <jim <at> meyering.net> wrote:
> On Fri, Aug 5, 2016 at 7:32 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
>>
>> On Fri, 5 Aug 2016 13:29:43 -0700
>> Jim Meyering <jim <at> meyering.net> wrote:
>>
>>> On Fri, Aug 5, 2016 at 4:30 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> 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:

[0001-tests-avoid-occasional-false-positive-on-OS-X.diff (text/plain, attachment)]

Message #22 received at 24159-done <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: Jim Meyering <jim <at> meyering.net>
Cc: 24159-done <at> debbugs.gnu.org
Subject: Re: bug#24159: [PATCH] dfa: minor fix for whether dfa is fast or not
Date: Sat, 06 Aug 2016 17:20:35 +0900

On Fri, 5 Aug 2016 22:02:31 -0700
Jim Meyering <jim <at> meyering.net> wrote:

> 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.

Thanks for exampination and push.  I ran tests on fedora 24 (GCC 6.1.1),
and all tests are passwd with before and after patch, and after patching
your 10 seconds patch in addition.

Norihiro

Message #25 received at 24159-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 24159-done <at> debbugs.gnu.org
Subject: Re: bug#24159: [PATCH] dfa: minor fix for whether dfa is fast or not
Date: Sat, 6 Aug 2016 09:29:12 -0700

[Message part 1 (text/plain, inline)]

On Fri, Aug 5, 2016 at 10:05 PM, Jim Meyering <jim <at> meyering.net> wrote:
> On Fri, Aug 5, 2016 at 10:02 PM, Jim Meyering <jim <at> meyering.net> wrote:
>> On Fri, Aug 5, 2016 at 7:32 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
>>>
>>> On Fri, 5 Aug 2016 13:29:43 -0700
>>> Jim Meyering <jim <at> meyering.net> wrote:
>>>
>>>> On Fri, Aug 5, 2016 at 4:30 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> 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:

[0001-tests-backref-multibyte-slow-avoid-false-positive.diff (text/plain, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.