att / ast

AST - AT&T Software Technology
Eclipse Public License 1.0
559 stars 152 forks source link

RFC: are non-forking subshells still worth having? #480

Open McDutchie opened 6 years ago

McDutchie commented 6 years ago

_(Please note: below I use the POSIX definition of "subshell", and not any other confused and misleading notion of "subshell" used in various shell manuals and tutorials. To quote: "A subshell environment shall be created as a duplicate of the shell environment, except that signal traps that are not being ignored shall be set to the default action. Changes made to the subshell environment shall not affect the shell environment. Command substitution, commands that are grouped with parentheses, and asynchronous lists shall be executed in a subshell environment. Additionally, each command of a multi-command pipeline is in a subshell environment; as an extension, however, any or all commands in a pipeline may be executed in the current environment.")_


All shells except ksh93 fork off a new process to create a subshell environment. ksh93's non-forking implementation of subshell environments has always been rife with problems. Even David Korn has never managed to create a cleanly separate execution environment without forking a new process.

One egregious issue is #73, which can silently cause the execution of the wrong code. Every ksh93 version has that bug. Other issues include #108 and #416.

478 is a recent regression related to non-forking subshells. In that issue, @siteshwar inserted a telling comment:

Unfortunately the code to handle input/output in subshells is too complicated and it's very likely that fixing each bug will create another regression.

I cannot but agree. I, for one, am unable to understand the ksh93 source code. It is very convoluted and there are hardly any comments. I have to say I have great respect for @krader1961 and @siteshwar for having taken this on.

Now, as far as I know, there are two original reasons why ksh93 implemented subshells without forking a process:

  1. Performance: forks used to be expensive.
  2. To run on systems without fork().

I don't think either of these still apply. Forks have been pretty cheap for a long time now. Even on Windows it's much better than it used to be. There are shells such as dash and FreeBSD sh that are faster than ksh93 while still forking their subshells. And good luck finding a system without fork() younger than 20 years. I assume you're not trying to be compatible with FreeDOS. :)

And of course ksh93 is already perfectly capable of forking off a subshell in a new process: by necessity it does so for a background job. In certain cases it also forks off a command substitution containing a redirection (see my comment on #478). And of course, forked subshells are always cleanly separate because the separation is enforced by the kernel.

Non-forking subshells were an interesting experiment conceived at AT&T that may have served a purpose for a time, but never worked quite right. Now that operating systems have matured, it seems to me that the need for them has gone away.

So here's a radical idea. Why not simply disable and then remove the whole non-forking subshell experiment and fork all subshells like other shells do? I think you'd see a lot of bugs simply disappear, and you'd be removing a maintenance nightmare as well.

What do you think?

krader1961 commented 6 years ago

👍 👍 👍 It would be interesting to see how hard it would be to disable this "feature". I'm especially curious how entwined it is with support for { ...; } blocks. Hopefully not at all but with this code who knows.

McDutchie commented 6 years ago

Since { ...; } does not create a subshell, it should not be intertwined at all, but yeah, who knows. I certainly don't.

McDutchie commented 6 years ago

One thing to keep in mind is the behaviour of the ${.sh.subshell} counter. It is increased when creating a non-forked subshell environment, but (rather bizarrely) it is reset to zero when actually forking off a subshell in a separate process, such as in a background job subshell. I don't know if that was intended or not, but in any case, if you decide to fork all subshells then it should count forked subshell levels in future.

dannyweldon commented 6 years ago

In my opinion, non-forking subshells are another killer feature of korn shell and I for one would not like to see it go. Removing it may cause significant script breakage.

Consider this code, which works for ksh and interestingly for zsh as well:

cat /etc/passwd | while IFS=: read userid pass uid gid gecos home shell; do [[ $userid == root ]] && break; done; echo $userid '=>' $shell

Code like that has always worked for korn shell, as far as I can remember; even for ksh88. It has never worked for bash, because everything after the pipe is run in a subshell, forcing one to use a heredoc with an embedded subcommand as a workaround, which is not very intuitive, unlike the above.

McDutchie commented 6 years ago

@dannyweldon: Actually, that is completely unrelated to non-forking subshells. The killer feature in question is that ksh88 and ksh93 do not execute the last element of a pipe in a subshell at all (forked or otherwise) and there is no reason why that would change if subshells are forked.

As you've observed, zsh does the same thing (and always has), and it forks its subshells. And ksh88 does the same thing too, even though non-forked subshells were not introduced until ksh93. These days, bash does the same thing too if job control is disabled and the lastpipe shell option is on.

edit: I did a little experimenting and it looks like ksh93 already uses a forked subshell for the first element of the pipe in a construct like ... | read or ... | while read. Which makes sense. How else could it possibly establish a pipe between it and the main shell process?

dannyweldon commented 6 years ago

Thanks @McDutchie, that's good to know.

edit: I did a little experimenting and it looks like ksh93 already uses a forked subshell for the first element of the pipe in a construct like ... | read or ... | while read. Which makes sense. How else could it possibly establish a pipe between it and the main shell process?

Yes, it still has to fork to run subprocesses, even if they are shell functions, I believe.

stephane-chazelas commented 6 years ago

All the advanced ("experimental") features of ksh93 (types, object oriented stuff, disciplines) more or less rely on that (or would be far less appealing without).

The main advantage is for things like $(myfunction) or $(builtin) (where it does a lot more than not forking).

(it also has the ${ cmd;} form of command substitution like fish's command substitution).

stephane-chazelas commented 6 years ago

Note that it's the other way round, forks used to be cheap but nowadays are very expensive. For most executed simple commands (think things like dirname, echo, printf), most of the time spent is in the forking on most systems (those with an effective vfork() are a lot better), even more than the loading of the executable, dynamic linking. Hence, the building-in of all ast-open utilities and the non-forking command substitution.

McDutchie commented 6 years ago

All the advanced ("experimental") features of ksh93 (types, object oriented stuff, disciplines) more or less rely on that (or would be far less appealing without).

I'll admit to not understanding that.

How do types, object oriented stuff, and disciplines depend on not forking subshells? How would forking subshells make them less appealing, or even affect them at all?

Isn't a discipline function simply a special kind of shell function whose execution is triggered when a corresponding variable is read or changed? Shell functions are normally executed in the current environment.

The main advantage is for things like $(myfunction) or $(builtin) (where it does a lot more than not forking).

That's interesting, I didn't know that. Could you elaborate on "a lot more"? Like what?

stephane-chazelas commented 6 years ago

The main advantage is for things like $(myfunction) or $(builtin) (where it does a lot more than not forking).

That's interesting, I didn't know that. Could you elaborate on "a lot more"? Like what?

The builtins in command substitutions simulate their output. In $(echo foo), echo doesn't write foo\n to a pipe that is read by the main process like in other shells. All that is bypassed with the would-be output making up the expansion directly. Processes are only forked to execute command (and that's when pipes are created to read their output which is then appends to the rest of the expansion). (or at least that's how it appears to work looking at strace output, I've not looked at the code).

See also https://unix.stackexchange.com/questions/430494/compare-command-output-inside-if-statement-without-subshell/430502#430502

How do types, object oriented stuff, and disciplines depend on not forking subshells? How would forking subshells make them less appealing, or even affect them at all?

I was mostly thinking of command substitution as mentioned above, which you'd heavily use if you were to use ksh93 object orienting facilities (types in ksh93 terminology). That's another poorly documented feature. see src/cmd/ksh93/tests/types.sh for examples. I've never really used it, but I did play with it a few years ago to try and see what one could do with it. I did said "experimental" above, because while playing with it, I saw quite a few bugs which makes me think probably not many people used it outside of bell labs.

In another words, if you're going to do some object oriented programming with ksh93, since the way to return data is with command substitution, once command substitution forks (which would make it thousands of times slower), that feature becomes less appealing.

stephane-chazelas commented 6 years ago

which would make it thousands of times slower

That's a bit of an exageration.

$ time ksh -c 'for ((i = 0; i<50000; i++)) { a=$(echo x); }'
ksh -c 'for ((i = 0; i<50000; i++)) { a=$(echo x); }'  0.69s user 0.24s system 99% cpu 0.936 total
$ time bash -c 'for ((i = 0; i<50000; i++)) { a=$(echo x); }'
bash -c 'for ((i = 0; i<50000; i++)) { a=$(echo x); }'  36.54s user 17.36s system 108% cpu 49.846 total

More like 50 times as slow (here on Linux amd64).

Note that the subshell simulation has quite a significant overhead:

$ time ksh -c 'for ((i = 0; i<50000; i++)) { a=${ echo x; }; }'
ksh -c 'for ((i = 0; i<50000; i++)) { a=${ echo x; }; }'  0.39s user 0.00s system 99% cpu 0.396 total

So where possible, you'd still want to avoid subshells even if they don't fork.

For completeness and to show that forking is indeed expensive compared to loading and initialising an external executable:

$ time bash -c 'for ((i = 0; i<50000; i++)) { a=$(/bin/echo x); }'
bash -c 'for ((i = 0; i<50000; i++)) { a=$(/bin/echo x); }'  94.33s user 43.52s system 108% cpu 2:07.43 total
McDutchie commented 6 years ago

The builtins in command substitutions simulate their output. In $(echo foo), echo doesn't write foo\n to a pipe that is read by the main process like in other shells. All that is bypassed with the would-be output making up the expansion directly.

But if that's the case, no subshell is created at all for that command substitution, forked or otherwise. It is treated as just another kind of expansion. So my reply would be the same one as the one to dannyweldon earlier -- this is completely unrelated, and forking subshells would not affect it.

I just discovered FreeBSD sh does the same for $(echo foo). And it forks its subshells.

I was wrong there. No it doesn't, not in that case.

stephane-chazelas commented 6 years ago

But if that's the case, no subshell is created at all for that command substitution,

Yes, the $(...) starts a subshell (as required by POSIX), ${ ...; } doesn't.

$ ksh93 -c 'b=1; a=$(b=2); echo "$b"'
1
$ ksh93 -c 'b=1; a=${ b=2; }; echo "$b"'
2

On FreeBSD, I find:

FB:~$ sh -c 'a=$(echo $((b=2))); echo "$a, $b"'
2,

So it must do some form of subshell emulation (here at least make the variables local to the subshell). I find that it still forks for cd or alias for instance, or if there's more than one command in the cmdsubst

McDutchie commented 6 years ago

Note that the subshell simulation has quite a significant overhead:

Thus limiting or even obliterating their performance benefit vs. forked subshells in real-world usage. Yes, I'd noticed that. (I emailed you an example of a performance comparison with dash.)

For completeness and to show that forking is indeed expensive compared to loading and initialising an external executable:

But you're doing a fork + exec instead of just a fork, so you're not proving anything regarding the relative cost of fork() vs. exec'ing and initialising an external command.

Also, bash is the slowest shell around by a large margin, so using it to prove your performance point is a bit unfair. :-)

A better way of measuring that would be to using a builtin, and then an external command, on a fast shell that always forks its command substitutions -- say, dash. What do you get for these?

On my laptop with macOS, I get:

$ time dash -c 'i=0; while [ $((i+=1)) -le 50000 ]; do a=$(echo x); done'
real    0m26.388s
user    0m9.871s
sys     0m16.066s

$ time dash -c 'i=0; while [ $((i+=1)) -le 50000 ]; do a=$(/bin/echo x); done'
real    1m42.554s
user    0m44.688s
sys     0m53.356s

So on MacOS, with a fork() that is as slow as an arthritic dog, launching an external command takes 4 times as long as a simple command substitution fork().

On Linux (Intel Xeon 4 core 3.3Ghz), I get:

$ time dash -c 'i=0; while [ $((i+=1)) -le 50000 ]; do a=$(echo x); done'
real    0m9.328s
user    0m1.142s
sys     0m4.929s

$ time dash -c 'i=0; while [ $((i+=1)) -le 50000 ]; do a=$(/bin/echo x); done'
real    1m20.039s
user    0m50.559s
sys     0m8.109s

That's a factor ten difference.

So yeah, I maintain my position that forks are cheap. :-)


edit: At the risk of adding noise, I just can't resist adding this. This is FreeBSD, with bash and dash from ports, on a VirtualBox VM, on the same Mac laptop that got the dog-slow results above...

$ time dash -c 'i=0; while [ $((i+=1)) -le 50000 ]; do a=$(echo x); done'
real    0m1.272s
user    0m0.222s
sys     0m1.045s

$ time dash -c 'i=0; while [ $((i+=1)) -le 50000 ]; do a=$(/bin/echo x); done'
real    0m3.158s
user    0m0.835s
sys     0m2.268s

A factor three difference, so still relatively cheap forks. But, holy blazing speed, Batman! What is the secret of the FreeBSD people?

stephane-chazelas commented 6 years ago

But you're doing a fork + exec instead of just a fork, so you're not proving anything regarding the relative cost of fork() vs. exec'ing and initialising an external command

I did, that was:

Showing that forks make cmdsubsts with builtins prohibitive and that the forking is significative compared to fork+exec. You have a point about bash being the slowest shells of all.

$ time dash -c 'i=0; while [ $((i+=1)) -le 50000 ]; do a=$(echo x); done'
real    0m9.328s
user    0m1.142s
sys     0m4.929s

So yeah, I maintain my position that forks are cheap. :-)

10s/50000 = 0.2ms to store "x" into a variable is not "cheap".

As discussed privately, ksh93 advertised goal was to be a programming language playing in the same field as perl and python.

The equivalent of:

sub f { return "x"; }
$var = f();

in ksh93 is

function f { print x; }
var=$(f)
# or var=${ f; } if you don't mind `f` potentially changing your environment

(yes, I know ksh93 can also pass variables by reference which is another option to return value).

kernigh commented 6 years ago

Here's a benchmark where ksh is much faster than other shells. It uses the slow recursive algorithm for the Fibonacci sequence; I adapted it from Ruby's sample/fib.rb. The syntax $(fib ...) works in more shells than ${ fib ...;}.

fib() {
    if test $1 -lt 2; then
        echo $1
    else
        echo $(( $(fib $(($1 - 2)) ) + $(fib $(($1 - 1)) ) ))
    fi
}
fib 20
$ type nksh
nksh is an alias for /home/kernigh/park/ast/build/src/cmd/ksh93/ksh
$ time nksh fib.sh
6765
    0m00.69s real     0m00.67s user     0m00.02s system
$ time dash fib.sh
6765
    0m15.65s real     0m01.42s user     0m12.26s system
$ time zsh fib.sh
6765
    0m18.88s real     0m01.86s user     0m15.92s system
$ time bash fib.sh
6765
    0m29.92s real     0m04.59s user     0m22.21s system
dannyweldon commented 6 years ago

BTW, It's helpful to put this at the end of all the above ksh test snippets:

echo subshells: ${.sh.stats.subshell} forks: ${.sh.stats.forks} spawns: ${.sh.stats.spawns}

eg.

ksh -c 'fib() { if test $1 -lt 2; then echo $1; else echo $(( $(fib $(($1 - 2)) ) + $(fib $(($1 - 1)) ) )); fi; }; fib 20; echo subshells: ${.sh.stats.subshell} forks: ${.sh.stats.forks} spawns: ${.sh.stats.spawns}'
6765
subshells: 21890 forks: 0 spawns: 0
krytarowski commented 6 years ago

Optimizing shell code by not calling fork(2) is no longer needed in the modern world.

McDutchie commented 6 years ago

I'll have to admit that fib() function is very fast for having created 21890 subshells. But Stéphane Chazelas points out that ${ command; } can be used as a command substitution executed in the current shell environment, and I see no reason why that should go away. So it can be rewritten as:

ksh -c 'fib() { if test $1 -lt 2; then echo $1; else echo $(( ${ fib $(($1 - 2)); } + ${ fib $(($1 - 1)); } )); fi; }; fib 20; echo subshells: ${.sh.stats.subshell} forks: ${.sh.stats.forks} spawns: ${.sh.stats.spawns}'
6765
subshells: 0 forks: 0 spawns: 0
McDutchie commented 6 years ago

I would also point out that this can be done much faster still with nothing but pure POSIX shell and utilities, and with only one fork:

ksh -c 'fib() { awk '\''function fib(n) { if (n<2) return n; else return fib(n-2)+fib(n-1); } BEGIN { print fib(ARGV[1]); }'\'' "$1"; }; fib 20; echo subshells: ${.sh.stats.subshell} forks: ${.sh.stats.forks} spawns: ${.sh.stats.spawns}'
6765
subshells: 0 forks: 1 spawns: 0

That takes 0.014s on my system. The ksh variant with ${ ... ; } takes 0.183s. The ksh variant with regular command substitutions takes 0.307s.

I think this demonstrates mainly the need to use the right tool for the job.

siteshwar commented 6 years ago

This is the kind of issue where I would have asked for advice from ksh's original creators. Unfortunately I have not managed to get response from any of them. Non-forking subshells have caused inconsistent behavior on different occasions, but removing them is going to be a major change. At this time I can only speculate how it is going to affect existing scripts. This is something that I would not target for the next release, but definitely we should consider afterwards.

McDutchie commented 6 years ago

Stéphane and others have been somewhat convincing in pointing out the advantages of fast command substitution subshells, altough I still think those arguments are a bit far-fetched and have limited relevance to real-world usage.

I'll concede, though, that non-forking command substitutions have a real advantage for some use cases. However, the question is, is that worth the bugs?

I'm particularly thinking of #73. Unsetting or redefining a function within a subshell is silently ignored. Silently executing the wrong code, in what universe is that acceptable? Frankly, I am gobsmacked that this bug has been present for the last 25 years and was never considered a release blocker.

There are all sorts of possible reasons to unset or redefine functions within subshells. For example, it is a quite conceivable to wrap some command in a function by the same name, e.g. ls(), and then execute the command from that function directly using a subshell: (unset -f ls; ls "$@"). Well, on ksh, the unset -f silently fails, so you've got infinite recursion.

(Yes, I know command ls is the canonical way to do this. The above should still just work and it's just a deliberately simple example anyway.)

Another possible use case is temporarily changing the behaviour of a program by replacing one of its functions with another by the same name within a subshell. That fails as well.

So the question the developers need to answer is not: is there a legitimate use case for non-forking subshells? but: is it worth the bugs? The answer depends on whether those bugs (especially #73) can be solved.

If you can't make it work properly for the next release, please at least make the shell print an error and exit when attempting to redefine or unset a function within a subshell. That would still be much preferable to executing the wrong code.

krader1961 commented 6 years ago

but: is it worth the bugs?

I need to take some time to read the earlier comments thoughtfully but my knee-jerk reaction is a resounding "no". Even if we fixed the problems with the existing non-fork subshell behavior it is not at all obvious the added code complexity, and potential source of new bugs, is worth the seemingly insignificant performance benefits. Correct behavior trumps other considerations such as performance. And while that is not always true it is true for a program like ksh.

siteshwar commented 6 years ago

So the question the developers need to answer is not: is there a legitimate use case for non-forking subshells? but: is it worth the bugs? The answer depends on whether those bugs (especially #73) can be solved.

Optimizations never worth bugs. There is nothing more non-efficient than a software that does not work. Unfortunately there are many places where the original developers have chosen optimizations over consistent behavior (for e.g. #468). If the code related to non-forking subshells can be removed without creating any side effects, I am fine with removing it.

McDutchie commented 6 years ago

If the code related to non-forking subshells can be removed without creating any side effects, I am fine with removing it.

It should be kept in mind that any such side effect is another bug, either in ksh or in the script using ksh.

Because, by definition (see POSIX definition quoted at the top of the thread), a subshell is a completely separate execution environment, initialised as a verbatim copy of the current shell, including non-exported variables, functions, etc., with the sole exception of traps which are reset in subshells.

Thus, any script that relies on the broken or incomplete separation of a subshell environment is relying on a bug, and is thereby itself broken.

stephane-chazelas commented 6 years ago

2018-04-25 13:03:13 +0000, Martijn Dekker: [...]

Thus, any script that relies on the broken or incomplete separation of a subshell environment is relying on a bug, and is thereby itself broken. [...]

Some other things that are impacted by a non-forking subshell:

$ ksh -c '(echo foo); echo done >&2' | : $ zsh -c '(echo foo); echo done >&2' | : done

For ksh, it's the main process that got the SIGPIPE because the "echo" (which is a builtin, yet another optimisation that affects the behaviour here, though that particular optimisation is done by most shells) was not done in a child process.

$ mksh -c 'printf foo; echo "$? done" >&2' | : 141 done $ dash -c 'printf foo; echo "$? done" >&2' | : $ zsh -c 'printf foo; echo "$? done" >&2' | : $ bash -c 'printf foo; echo "$? done" >&2' | :

(in the case of printf, mksh is the one that behaves as expected as it doesn't do the optimisation (doesn't have printf built in)).

Also on limits:

$ ksh -c 'ulimit -t 1; (while :; do :; done); echo done' zsh: killed ksh -c 'ulimit -t 1; (while :; do :; done); echo done' $ zsh -c 'ulimit -t 1; (while :; do :; done); echo done' done

Though, if I move the ulimit inside the subshell, that's better:

$ ksh -c 'echo $$; (ulimit -t 1; while :; do :; done); echo done' 3581 ksh: 3582: Killed done

There was a fork there. So it looks like it's a cheap way to force a fork (ulimit -t unlimited also forces a fork AFAICT).

-- Stephane

McDutchie commented 6 years ago

Thank you for discovering a way to force a fork! So far I only discovered that including a redirection in a command substitution forces that command substitution to be forked in certain cases.

edit: Hmm... bltins/ulimit.c contains this interesting line:

if (shp->subshell && !shp->subshare) sh_subfork();

Could sh_subfork() be used in bltins/typeset.c to fix #73?

McDutchie commented 6 years ago

So now that it's known that there is a simple way to force a fork at any point during the execution of a subshell, my opinion on this issue has changed. I now think you should probably keep the non-forking subshells, but be much more conservative about forcing a fork using sh_subfork() than David Korn was.

Any time things like functions, traps, or aliases are set, unset or changed in a subshell, or the state of that subshell is drastically altered in some other way, you should fork. This way, things can hopefully be made robust while keeping the vast majority of the performance advantage.

krytarowski commented 6 years ago

BTW. K.R.E. had ideas about vfork(2) with a kick-the-parent syscall option. It might be like an intermediate solution.

krader1961 commented 6 years ago

@krytarowski, Can you provide more detail? I can't grok your comment. We also, recently, dropped support for vfork() because on some platforms it doesn't have the legacy behavior.

@McDutchie, I would prefer to simplify the code rather than complicate it with more ad-hoc special cases. I haven't yet found the time to carefully consider the performance arguments for a non-forking subshell. But I'll bet that the performance benefits for the 98'th percentile of ksh scripts are so small that the added complexity, and potential bugs, can't be justified. Yes, there are undoubtedly ksh scripts that cache so much data they benefit from the optimization. But I don't see how catering to those atypical scripts at the expense of all the other use cases (in particular the interactive use case) can be justified.

Too many people seem to believe that just because an optimization is theoretically possible it is worth implementing regardless of the cost. Costs that are both immediate and long term.

dannyweldon commented 6 years ago

I would also point out that this can be done much faster still with nothing but pure POSIX shell and utilities, and with only one fork

I thought I would try it as a math function to see if it was faster, but came across two issues. As a single liner, I had to add a newline after the math function opening brace:

ksh -c 'function .sh.math.fib f {
if (( $f < 2 )); then .sh.value=$f; else .sh.value=$(( fib($f - 2) + fib($f - 1) )); fi; }; echo $(( fib(8) )); echo subshells: ${.sh.stats.subshell} forks: ${.sh.stats.forks} spawns: ${.sh.stats.spawns}'
21
subshells: 0 forks: 0 spawns: 0

Otherwise I get a syntax error:

ksh: syntax error at line 1: `((' unexpected

Also, if I go any higher, say fib(9), I get a recursion error, such as:

line 2:  0 < 2 : recursion too deep

which is odd considering that the previous non-math versions work just fine.

krytarowski commented 6 years ago

@krader1961 http://mail-index.netbsd.org/tech-kern/2018/04/03/msg023301.html

krader1961 commented 6 years ago

FWIW, I have carefully re-read this issue and am still unconvinced that the current behavior that tries to avoid forking subshells is worth the complexity and bugs. As I said earlier even if the current bugs with respect to this feature were fixed the complexity all but guarantees new bugs would occur in the future as the code evolves. The POSIX 1003 standard for shells like ksh makes it effectively (as opposed to hypothetically) impossible to avoid forking for constructs like $(...). Correctness of behavior trumps performance.

Yes, given unlimited resources the optimization can be made to work. AT&T didn't make it work. And I don't see anyone else stepping up to commit those resources.

Too, if you're using ksh to do something like compute the fibonacci sequence, and expect it to perform within an order of magnitude of other implementations in languages like Python or C, you're using the wrong tool.

krader1961 commented 6 years ago

There is a bug in nv_putval() that manifests on macOS as a SIGSEGV. But only when when the code is executed in a subcommand, $(...), context. I don't know that this is because of the non-forking subshell optimization being discussed in this issue but it is a possibility and it wouldn't surprise me in the least if disabling that optimization fixed that failure.

siteshwar commented 6 years ago

@cococlyde There are some comments in favor of and against non-forking subshells in this thread. I would like to hear your thoughts on it. Key questions are :

siteshwar commented 5 years ago

We hit another issue caused by non-forking subshells in #9. I want to selectively start removing code blocks like this. It should be safe to do so, and would save us from a few (or more) bug reports.

krader1961 commented 5 years ago

I hate to add another comment to an issue that is already way too long but a bug I documented in issue #1074 needs to be duplicated here rather than just as a link. Precisely because it is a crystal clear example why this sort of optimization is problematic.

This statement

https://github.com/att/ast/blob/e756d5b57410fff4db657cdf9f4d94ff2a8958f3/src/cmd/ksh93/sh/subshell.c#L582

is broken as the condition can never be true since sh_state(SH_NOFORK) always returns a non-zero value. Thus the sh_onstate(shp, SH_NOFORK) will never be executed. So the question is what was intended? This is one of only two places where sh_onstate(shp, SH_NOFORK) appears (the other is in xec.c).

There are so many long standing (i.e., more than a decade old) bugs involving this optimization that I find it difficult to justify the performance improvement it provides. Notwithstanding the fact I have spent a lot of my professional life dealing with performance problems above and beyond what any competent software engineer would be concerned about. As I and others have stated in previous comments correct behavior is far more important than better performance. Especially since something on the order of 90% of ksh scripts are not sensitive to the performance of $(...) subshells.

Yes, I pulled the 90% figure in the previous paragraph out of my ass. I think the value is far higher. I will eat a printed version of this issue if someone can show that at least 10% of a random sampling of ksh scripts would be slowed down by removing the no-fork subshell optimization to an extent that anyone would complain.

This is ultimately about whether ksh can be made a general purpose language despite its origins as a POSIX shell (e.g., Bourne shell) language. I think the answer is "no" given the idiosyncratic behavior of shells that adhere to that standard.