Skip to content

Commit

Permalink
[osh/cmd_exec] Optimize the number of processes started.
Browse files Browse the repository at this point in the history
command.Simple now has a do_fork attribute which can be turned off.

Add more test cases.  Addresses issue #254.

Score:

- Our "score" is 60, compared with 78 for bash (the max) and 57 for yash
  (the min).
- We beat zsh which has a score of 62.
- We no longer start more processes than bash in any case.  We start
  FEWER in 17 out of 30 cases!
- Oil isn't the minimum in 4 out of 30 cases.  There's still at least
  one of these I think we should handle.

Other trivia:

- We lost the 'command date | wc -l' optimization, but no other shell
  does this!
- bash starts TWO more processes than other shells, for case 28.
- We also behave like yash in optimizing the same way whether we get -c,
  stdin, or file input.  Other shells only optimize -c.  TODO: publish
  these tests.
  • Loading branch information
Andy Chu committed Mar 29, 2020
1 parent 664c898 commit bcb0da5
Show file tree
Hide file tree
Showing 10 changed files with 283 additions and 68 deletions.
2 changes: 1 addition & 1 deletion bin/oil.py
Expand Up @@ -711,7 +711,7 @@ def ShellMain(lang, argv0, argv, login_shell):

_tlog('Execute(node)')
try:
status = main_loop.Batch(ex, c_parser, arena, nodes_out=nodes_out)
status = main_loop.Batch(ex, c_parser, arena, nodes_out=nodes_out, is_main=True)
if ex.MaybeRunExitTrap():
status = ex.LastStatus()
except util.UserExit as e:
Expand Down
15 changes: 10 additions & 5 deletions core/main_loop.py
Expand Up @@ -21,6 +21,7 @@
from core import error
from core import ui
from core import util
from core.util import log

from typing import Any, Optional, List, TYPE_CHECKING
if TYPE_CHECKING:
Expand All @@ -31,6 +32,8 @@
from osh.cmd_exec import Executor
from osh.prompt import UserPlugin

_ = log


def Interactive(opts, ex, c_parser, display, prompt_plugin, errfmt):
# type: (Any, Executor, CommandParser, _IDisplay, UserPlugin, ErrorFormatter) -> int
Expand Down Expand Up @@ -118,8 +121,8 @@ def Interactive(opts, ex, c_parser, display, prompt_plugin, errfmt):
return status


def Batch(ex, c_parser, arena, nodes_out=None):
# type: (Any, CommandParser, Arena, Optional[List[command_t]]) -> int
def Batch(ex, c_parser, arena, nodes_out=None, is_main=False):
# type: (Any, CommandParser, Arena, Optional[List[command_t]], bool) -> int
"""Loop for batch execution.
Args:
Expand Down Expand Up @@ -161,9 +164,11 @@ def Batch(ex, c_parser, arena, nodes_out=None):
nodes_out.append(node)
continue

#log('parsed %s', node)
# Only optimize if we're on the last line like -c "echo hi" etc.
optimize = is_main and c_parser.line_reader.LastLineHint()

is_return, is_fatal = ex.ExecuteAndCatch(node)
# can't optimize this because we haven't seen the end yet
is_return, is_fatal = ex.ExecuteAndCatch(node, optimize=optimize)
status = ex.LastStatus()
# e.g. 'return' in middle of script, or divide by zero
if is_return or is_fatal:
Expand All @@ -176,7 +181,7 @@ def ParseWholeFile(c_parser):
# type: (CommandParser) -> command_t
"""Parse an entire shell script.
This uses the same logic as Batch().
For osh -n. This uses the same logic as Batch().
"""
children = []
while True:
Expand Down
3 changes: 2 additions & 1 deletion core/process.py
Expand Up @@ -701,7 +701,8 @@ def Run(self):
self.ex.mutable_opts.errexit.Disable()

try:
self.ex.ExecuteAndCatch(self.node, fork_external=False)
# optimize to eliminate redundant subshells like ( echo hi ) | wc -l etc.
self.ex.ExecuteAndCatch(self.node, optimize=True)
status = self.ex.LastStatus()
# NOTE: We ignore the is_fatal return value. The user should set -o
# errexit so failures in subprocesses cause failures in the parent.
Expand Down
16 changes: 16 additions & 0 deletions frontend/reader.py
Expand Up @@ -44,6 +44,14 @@ def Reset(self):
"""Called after command execution in main_loop.py."""
pass

def LastLineHint(self):
# type: () -> bool
"""A hint if we're on the last line, for optimization.
This is only for performance, not correctness.
"""
return False


class DisallowedLineReader(_Reader):
"""For CommandParser in Oil expressions."""
Expand All @@ -69,15 +77,23 @@ def __init__(self, f, arena):
"""
_Reader.__init__(self, arena)
self.f = f
self.last_line_hint = False

def _GetLine(self):
# type: () -> Optional[str]
line = self.f.readline()
if len(line) == 0:
return None

if not line.endswith('\n'):
self.last_line_hint = True

return line

def LastLineHint(self):
# type: () -> bool
return self.last_line_hint


def StringLineReader(s, arena):
# type: (str, Arena) -> FileLineReader
Expand Down
14 changes: 10 additions & 4 deletions frontend/syntax.asdl
Expand Up @@ -254,8 +254,11 @@ module syntax

command =
NoOp
-- NOTE: block is always a BraceGroup.
| Simple(word* words, redir* redirects, env_pair* more_env, command? block)
-- notes:
-- * block is always a BraceGroup
-- * do_fork is semantic, not syntactic
| Simple(word* words, redir* redirects, env_pair* more_env, command? block,
bool do_fork)
-- This doesn't technically belong in the LST, but it's convenient for
-- execution
| ExpandedAlias(command child, redir* redirects, env_pair* more_env)
Expand Down Expand Up @@ -289,8 +292,11 @@ module syntax
| Case(word to_match, case_arm* arms, redir* redirects)
| ShFunction(string name, command body)
| TimeBlock(command pipeline)
-- Most nodes optimize it out as command*, but there are a few places where
-- this is useful for type safety.
-- Some nodes optimize it out as command*, but we use this for
-- 1. the top level
-- 2. ls ; ls & ls (same line)
-- 3. command_sub -- single child that's a CommandList
-- 4. Subshell -- single child that's a CommandList
| CommandList(command* children)

-- Oil stuff
Expand Down
2 changes: 1 addition & 1 deletion frontend/syntax_abbrev.py
Expand Up @@ -103,7 +103,7 @@ def _braced_var_sub(obj):
def _command__Simple(obj):
# type: (command__Simple) -> hnode_t
p_node = runtime.NewRecord('C')
if obj.redirects or obj.more_env or obj.block:
if obj.redirects or obj.more_env or obj.block or obj.do_fork == False:
return None # we have other fields to display; don't abbreviate

p_node.abbrev = True
Expand Down

0 comments on commit bcb0da5

Please sign in to comment.