Re: igncr vs text mode mounts, performance vs compatibility
Rob Walker wrote: > I looked into my scripts a little harder, have better results, some new > conclusions: I think you are missing the point somewhat. The thing you need to benchmark against is the older bash version before the 'igncr' option even existed, which read every script one byte at a time regardless of mount type or line endings. With typical 'configure' scripts easily exceeding 200 kB (and some more than 2.5 MB!), this resulted in massive overhead. *That* was the performance hit that motivated this whole ordeal in the first place. I understand you are advocating for igncr being set by default, but I got the impression that everyone agreed that this would probably be a good idea, and that Eric would probably make this the default eventually. Brian -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
Brian Dessent wrote: Rob Walker wrote: I looked into my scripts a little harder, have better results, some new conclusions: I think you are missing the point somewhat. The thing you need to benchmark against is the older bash version before the 'igncr' option even existed, which read every script one byte at a time regardless of mount type or line endings. With typical 'configure' scripts easily exceeding 200 kB (and some more than 2.5 MB!), this resulted in massive overhead. *That* was the performance hit that motivated this whole ordeal in the first place. I understand you are advocating for igncr being set by default, but I got the impression that everyone agreed that this would probably be a good idea, and that Eric would probably make this the default eventually. Indeed. If I were to hazard a guess, I would say that every possible angle of CRLF handling has been covered in one thread or another in the last month or so. Let's leave this all in Eric's capable hands. I'm confident that whatever he comes up with will be better than where we started and a vast improvement overall for everyone. From what I've seen so far, and what Rob's results would confirm, is that the changes made already are a giant leap forward. Obviously if there turns out to be a flaw in the delivered result, I am also very confident that someone on this list will find it and report it. Until then, we can all bask in the bliss of an issue well-covered and the lack of a need for further email on the subject. :-) -- Larry Hall http://www.rfk.com RFK Partners, Inc. (508) 893-9779 - RFK Office 216 Dalton Rd. (508) 893-9889 - FAX Holliston, MA 01746 -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
On Wed, Oct 18, 2006 at 09:50:00PM -0400, Larry Hall (Cygwin) wrote: >Brian Dessent wrote: >>Rob Walker wrote: >> >>>I looked into my scripts a little harder, have better results, some new >>>conclusions: >> >>I think you are missing the point somewhat. The thing you need to >>benchmark against is the older bash version before the 'igncr' option >>even existed, which read every script one byte at a time regardless of >>mount type or line endings. With typical 'configure' scripts easily >>exceeding 200 kB (and some more than 2.5 MB!), this resulted in massive >>overhead. *That* was the performance hit that motivated this whole >>ordeal in the first place. >> >>I understand you are advocating for igncr being set by default, but I >>got the impression that everyone agreed that this would probably be a >>good idea, and that Eric would probably make this the default >>eventually. > > >Indeed. If I were to hazard a guess, I would say that every possible >angle of CRLF handling has been covered in one thread or another in the >last month or so. Let's leave this all in Eric's capable hands. I'm >confident that whatever he comes up with will be better than where we >started and a vast improvement overall for everyone. From what I've seen >so far, and what Rob's results would confirm, is that the changes made >already are a giant leap forward. Obviously if there turns out to be a >flaw in the delivered result, I am also very confident that someone on >this list will find it and report it. Until then, we can all bask in >the bliss of an issue well-covered and the lack of a need for further >email on the subject. :-) I agree. We certainly don't need any more email on the subject. More email would be overkill. cgf -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
Thanks for reading, Brian. Apologies to all that my recent tend to bulk. The point of my post was to advocate igncr as the default for bash 3. I realize I'm not alone in this advocacy. I'm truly happy that bash 3 is super fast compared to bash 2, but bash 3's "incompatibility" is currently a (possibly serious for me) stumbling block. What motivated today's work was the bash 3.2 announcement, which (apparently) doesn't make igncr the default. Eric, what's your current thinking on this topic? -Rob Brian Dessent wrote: Rob Walker wrote: I looked into my scripts a little harder, have better results, some new conclusions: I think you are missing the point somewhat. The thing you need to benchmark against is the older bash version before the 'igncr' option even existed, which read every script one byte at a time regardless of mount type or line endings. With typical 'configure' scripts easily exceeding 200 kB (and some more than 2.5 MB!), this resulted in massive overhead. *That* was the performance hit that motivated this whole ordeal in the first place. I understand you are advocating for igncr being set by default, but I got the impression that everyone agreed that this would probably be a good idea, and that Eric would probably make this the default eventually. Brian -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/ -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Rob Walker on 10/18/2006 6:38 PM: > I looked into my scripts a little harder, have better results, some new > conclusions: Rob, please avoid http://cygwin.com/acronyms/#TOFU. Thanks for calculating some timings. > > - > line ending | mount mode | igncr | "user" time > - > CRLF | text | set | 1.0114s Here, both cygwin and bash are checking for \r (obviously, the bash check won't find any), and bash is forced to read the file one byte at a time. > - > CRLF | text | clear | 0.984s Slightly faster; bash is still forced to read one byte at a time, but it is not wasting efforts checking for \r. This matches the bash-3.1-6 behavior, regardless of mount point. > - > LF | text | set | 0.56995s > - > LF | text | clear | 0.5653s For these two, now bash can read a buffer at a time. OK, so bash's check for \r is in the noise compared to the speed penalty for slower file reads. > - > CRLF | bin | set | 0.59435s When bash must filter \r, the timing is still noticeable, making it even slower than a text mount that need not filter \r. > - > CRLF | bin | clear | whoops! > - > LF | bin | set | 0.5545s > - > LF | bin | clear | 0.5576s Indeed, as I predicted, LF only on binary mounts are as fast as you can get; the minor difference here on igncr is probably due to statistical variance. > > In the bin mode section (the Cygwin recommended mount mode): note here > that there's an approx 7% penalty between the most accomodating case > (CRLF on a binmode mount with igncr set) and the most restrictive case > (LF only on a bin mode mount with igncr clear). Less than 10% penalty > on this perverse benchmark (handling _nothing_ but linefeeds) seems like > a small price for compatibility. But there's also the issue of POSIX compatibility - ignoring \r is not POSIX compatible. And any speed penalty, however slight, that it noticeable in a benchmark, even if the penalty is in the noise for real life cases, is worth addressing - if everyone took the attitude that their patch was only 10% worse in the worst case, we'd have some slow programs. On the other hand, the complaint factor on the mailing list is a tangible factor, although much harder to objectively measure. If I make igncr the default (or more likely, if I make it depend on the state of POSIXLY_CORRECT), I will be noticeably saving myself time by not having to plow through so many emails from clueless users wondering why their CRLF scripts don't work on cygwin, since those same scripts won't work on Linux either. >> >> Are you saying that these people expect bash to treat CRLF as if the >> CR were non-whitespace? Can you give me an example where this would >> be a useful feature? It may not be a well-used feature, but I won't go so far as to call it not useful. One possible use - a script written with \n line endings, but which wants to intentionally generate an output file with \r\n line endings (this sounds like something sharutils might want to do). On Linux, literal \r in a here-doc get output to the file. So it stands to reason that someone might want to do the same action on cygwin when using a binary mount. Since cygwin's goal is to provide a Linux emulation, I don't see any reason to artificially limit cygwin by making bash always ignore \r; rather, I think it is only safe to ignore \r when explicitly told to do so (either by a text mount, or by using igncr). - -- Life is short - so eat dessert first! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.2.1 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFFN2y+84KuGfSFAYARAvs/AKDB1KWuMvOwVL7a2XRqapHpI0kO4QCeIv5U dhd/hrxm0UJUf1Cs0F0OFF4= =pKZR -END PGP SIGNATURE- -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
> >> Are you saying that these people expect bash to treat CRLF as if the > >> CR were non-whitespace? Can you give me an example where this would > >> be a useful feature? > > It may not be a well-used feature, but I won't go so far as to call it not > useful. One possible use - a script written with \n line endings, but > which wants to intentionally generate an output file with \r\n line > endings (this sounds like something sharutils might want to do). On > Linux, literal \r in a here-doc get output to the file. So it stands to > reason that someone might want to do the same action on cygwin when using > a binary mount. Since cygwin's goal is to provide a Linux emulation, I > don't see any reason to artificially limit cygwin by making bash always > ignore \r; rather, I think it is only safe to ignore \r when explicitly > told to do so (either by a text mount, or by using igncr). Just a thought, it would probably solve 99% of people's problems if you just specified that if the first line of the script ends in \r\n, then \r will be ignored for the rest of the file. Then you would just need to read the first line a byte at a time, and every subsequent line could be read efficiently whenever possible, right? And it seems unlikely that this could possibly break anything. -Lewis -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Lewis Hyatt on 10/24/2006 12:57 PM: > Just a thought, it would probably solve 99% of people's problems if you just > specified that if the first line of the script ends in \r\n, then \r will be > ignored for the rest of the file. Then you would just need to read the first > line a byte at a time, and every subsequent line could be read efficiently > whenever possible, right? And it seems unlikely that this could possibly break > anything. Propose a patch, and I will consider it. In my opinion, it was much easier to do igncr as an all or none option than it was to parse the first line and discard \r on a per-file basis, not to mention that all-or-none is easily configurable so that those of us who WANT literal \r as required by POSIX can do so. To date, I have been the only person proposing patches in the area of \r\n handling, rather than just ideas (which is probably why I am the maintainer of the cygwin bash port). And I have no personal interest in redoing this patch to a more complex algorithm at this time. - -- Life is short - so eat dessert first! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.2.1 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFFPqk884KuGfSFAYARAvGAAJ9FSIRGBQlf/scMHFle+dR59NsnZwCgwgHQ LqtTw7GlAnOllAcAHnmowI4= =DY8g -END PGP SIGNATURE- -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
RE: igncr vs text mode mounts, performance vs compatibility
> From: Eric Blake > Sent: Tuesday, October 24, 2006 7:03 PM > Subject: Re: igncr vs text mode mounts, performance vs compatibility > > According to Lewis Hyatt on 10/24/2006 12:57 PM: > > Just a thought, it would probably solve 99% of people's problems if > > you just specified that if the first line of the script > ends in \r\n, > > then \r will be ignored for the rest of the file. Then you > would just > > need to read the first line a byte at a time, and every subsequent > > line could be read efficiently whenever possible, right? > And it seems > > unlikely that this could possibly break anything. > > Propose a patch, and I will consider it. In my opinion, it > was much easier to do igncr as an all or none option than it > was to parse the first line and discard \r on a per-file > basis, not to mention that all-or-none is easily configurable > so that those of us who WANT literal \r I'm just curious here: *Why* do you (or anybody else) want bash to not ignore \r's (or better stated, to only understand The One True Text File Format (Whatever That Is)(tm))? I keep trying to figure out what is going to break when bash suddenly is able to understand \r\n as well as \n, and keep coming up empty. Furthermore, I don't recall a single instance of anybody coming to the list with a problem that was due to bash ignoring \r's (when it used to do so). > as required by POSIX > can do so. Is this the reason? If so, do you know why POSIX requires this? At some point POSIX compliance ceased to be a goal of the Cygwin project, so I don't see that as an argument either way. -- Gary R. Van Sickle -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
> Propose a patch, and I will consider it. In my opinion, it was much
> easier to do igncr as an all or none option than it was to parse the first
> line and discard \r on a per-file basis, not to mention that all-or-none
> is easily configurable so that those of us who WANT literal \r as required
> by POSIX can do so. To date, I have been the only person proposing
> patches in the area of \r\n handling, rather than just ideas (which is
> probably why I am the maintainer of the cygwin bash port). And I have no
> personal interest in redoing this patch to a more complex algorithm at
> this time.
I can understand your point of view, I'm sure it is a huge pain to deal with
this and then have to answer all the questions and complaints from people who
don't know why their scripts broke.
Given the number of support requests on the mailing list, it definitely seems
worthwhile to have this issue handled more transparently. The most compelling
argument for not translating \r\n, as I understand it, is that searching for the
\r slows down processing of large scripts, such as configure, when it is not
necessary. On the other hand, most user-created scripts that do contain \r\n
line endings are not large enough for the overhead to be problematic. For that
reason, I think it makes the most sense to enable or disable igncr on a per-file
basis. The other obvious alternative would be to have igncr turned on by
default, but then users would have to explicitly disable it to get the
performance enhancement when running configure scripts, and most would simply
not know they needed to do it.
Below is a patch to shell.c which implements my suggestion (It is just the
output of diff -u, is that OK? The patch is relative to the 3.2-4 version.) I
just modified the open_shell_script() function; now after it reads in 80
characters to make sure the script isn't binary, it also scans that buffer for
the first \n it finds and if it is preceded by \r, then it enables igncr. If
necessary it reads more characters, 80 bytes at a time. In most cases the
performance decrease for this check will be entirely negligible, particularly
since the first line of the script is almost always going to just be the
#!/bin/bash line anyway.
The only downside I can see to this is that bash will behave differently
depending whether it is reading from a file or from a pipe. But that is already
true in other cases as well, for instance the check for binary files is not
performed when reading from a pipe. It would be possible to extend the \r\n
check to piped input as well, although it would be somewhat more annoying to
implement.
If you think this overall concept is a good idea, I am happy to take everyone's
suggestions and implement them. It's a bit of a struggle for me to write C code
instead of C++, but I can give it a shot.
-Lewis
-
--- shell.c 2006-10-25 15:40:53.62500 -0400
+++ shell_patched.c 2006-10-25 15:41:02.09375 -0400
@@ -1335,6 +1335,7 @@
open_shell_script (script_name)
char *script_name;
{
+ extern int igncr;
int fd, e, fd_is_tty;
char *filename, *path_filename, *t;
char sample[80];
@@ -1426,8 +1427,32 @@
internal_error ("%s: cannot execute binary file", filename);
exit (EX_BINARY_FILE);
}
+
+ /* Now scan the first line of the file, if it ends with \r\n, then
+ enable the igncr shell option. */
+
+ while(sample_len>1)
+ {
+ int i;
+ for(i=0; i!=sample_len; ++i) if(sample[i]=='\n')
+ {
+ if(i>0 && sample[i-1]=='\r') igncr=1;
+ break;
+ }
+ if(ihttp://cygwin.com/ml/#unsubscribe-simple
Problem reports: http://cygwin.com/problems.html
Documentation: http://cygwin.com/docs.html
FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
According to Lewis Hyatt on 10/25/2006 1:48 PM:
> Below is a patch to shell.c which implements my suggestion (It is just the
> output of diff -u, is that OK? The patch is relative to the 3.2-4 version.) I
> just modified the open_shell_script() function; now after it reads in 80
> characters to make sure the script isn't binary, it also scans that buffer for
> the first \n it finds and if it is preceded by \r, then it enables igncr. If
> necessary it reads more characters, 80 bytes at a time. In most cases the
> performance decrease for this check will be entirely negligible, particularly
> since the first line of the script is almost always going to just be the
> #!/bin/bash line anyway.
Wow - someone besides me actually wrote a patch! What a welcome relief in
this thread.
>
> The only downside I can see to this is that bash will behave differently
> depending whether it is reading from a file or from a pipe. But that is
> already
> true in other cases as well, for instance the check for binary files is not
> performed when reading from a pipe. It would be possible to extend the \r\n
> check to piped input as well, although it would be somewhat more annoying to
> implement.
When reading from a pipe, the presence or absence of \r is determined by
CYGWIN=binmode (default) or CYGWIN=nobinmode (which is less tested, and
has been documented as breaking things like 'tar xjf foo.tar.bz2'). In
bash-3.2-4, if igncr is set, then even using CYGWIN=binmode and piping in
\r will ignore the \r, whereas you are correct that your patch would not
autodetect the \r line endings. For what I can think of, to check whether
the first line of piped input ends in \r\n, you would have to add a flag
that you are reading from a pipe and have not yet seen a line ending, then
check that flag for every read in input.c; which adds overhead to every
byte read, rather than just the first line. You can't afford to rewind,
or read in advance, because you don't know what is feeding the other end
of the pipe.
>
> If you think this overall concept is a good idea, I am happy to take
> everyone's
> suggestions and implement them. It's a bit of a struggle for me to write C
> code
> instead of C++, but I can give it a shot.
There are a few other things to think about with your initial attempt.
Right now, igncr is an all or nothing setting, can be inherited into child
processes via SHELLOPTS, but can only be changed by explicit user action.
Your patch only ever turns it on without user intervention, not off. You
are changing the global igncr variable even though you described your goal
as a per-file setting; so if bash reads two files in a row, first with
\r\n, and the second with \n only, the second inherits the fact that the
first decided to turn on igncr. At which point, why not just make igncr
the default to begin with? So it sounds like the effort to make a
per-file decision work correctly is getting bigger and bigger, with less
and less perceivable benefits.
>
> -Lewis
>
> -
>
> --- shell.c 2006-10-25 15:40:53.62500 -0400
> +++ shell_patched.c 2006-10-25 15:41:02.09375 -0400
> @@ -1335,6 +1335,7 @@
> open_shell_script (script_name)
> char *script_name;
> {
> + extern int igncr;
My uses of igncr in 3.2-4 are protected by #if __CYGWIN__, so that my
patches will still compile on other platforms; you would need to do
likewise to avoid a link error. Actually, when I propose an upstream
patch, I will probably do it in the form of ./configure --enable-igncr,
then protect all uses of igncr by IGNCR, so that the presence of igncr is
no longer cygwin-dependent, but can still be compiled out when desiring
strict POSIX conformance.
>int fd, e, fd_is_tty;
>char *filename, *path_filename, *t;
>char sample[80];
> @@ -1426,8 +1427,32 @@
> internal_error ("%s: cannot execute binary file", filename);
> exit (EX_BINARY_FILE);
> }
> +
> + /* Now scan the first line of the file, if it ends with \r\n, then
> +enable the igncr shell option. */
> +
> + while(sample_len>1)
> + {
> + int i;
> + for(i=0; i!=sample_len; ++i) if(sample[i]=='\n')
It seems a bit redundant to loop over the sample once in
check_binary_file, then again in your patch; I wonder if it is worth
patching check_binary_file instead?
Your code formatting is not consistent with nearby code; while this is not
a serious drawback, it makes it harder for me to propose your code to the
upstream maintainer, should I choose to accept something like it.
> + {
> + if(i>0 && sample[i-1]=='\r') igncr=1;
> + break;
> + }
> + if(i +
> + /* else need to read more data */
> + sample_len = read(fd, sample, sizeof(sample));
> + if(sample_len<
Re: igncr vs text mode mounts, performance vs compatibility
> There are a few other things to think about with your initial attempt. > Right now, igncr is an all or nothing setting, can be inherited into child > processes via SHELLOPTS, but can only be changed by explicit user action. > Your patch only ever turns it on without user intervention, not off. You > are changing the global igncr variable even though you described your goal > as a per-file setting; so if bash reads two files in a row, first with > \r\n, and the second with \n only, the second inherits the fact that the > first decided to turn on igncr. At which point, why not just make igncr > the default to begin with? So it sounds like the effort to make a > per-file decision work correctly is getting bigger and bigger, with less > and less perceivable benefits. I see, I misunderstood the behavior of shell options like that, I thought it only applied to the current bash process. It would be straightforward just to create a new global variable instead of using igncr, and to handle your other points, but in any case it sounds like you have made an effort to keep bash POSIX compliant, and we should just respect that behavior and rely on setting igncr as the default. -lewis -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
Eric Blake wrote: There is also the possibility of making bash turn igncr on by default, but /bin/sh leaving it off, since only /bin/sh is specified by POSIX; but that also gives me the willies thinking about people who will complain why their script doesn't work when they change from #!/bin/bash to #!/bin/sh. With all due respect, /bin/sh is meant to be POSIX-conforming, and IMO should force igncr to be off. IOW such people deserve what they get. :-) The other side of this argument is 'why did you switch to #!/bin/sh?'. Chance are it was done to make a script "more portable", and as we of course know '\r' line endings are *not* portable. Not that I'm trying to convince you of anything, just trying to help you feel better if you decide to make this decision. :-) -- Matthew $ kill bill - kill: can't find process "bill" -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
Re: igncr vs text mode mounts, performance vs compatibility
One additional thing I'd like to say, if people are considering possibly making a decision based off the line ending of the first line of the file: it's worth bearing in mind that it's quite possible for a file to end up with mixed line endings. Most editors are at least smart enough to convert the whole file to their preffered format, but not all, and there's always the possibility of nastiness like: C:\TEMP> echo "extra line" >> file_with_unix_line_endings It's yet another special case to think of: "Why my scrpt not wrok?" "Because it is on a binary mount && you didn't set igncr && you have CRLF line endings on some lines && the first line doesn't have a CRLF" It'll be one of those things that works 99% of the time for 99% of people, and by the time it bites someone they'll have forgotton all about this issue and will be very confused. -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://cygwin.com/docs.html FAQ: http://cygwin.com/faq/
