Fix macOS/Bash 3.2 breakage; eliminate subshells from exec-test, preprocess #8

mbland · 2017-09-27T10:22:25Z

These changes produce some substantial performance benefits across all platforms, but most especially on Windows. I'm using them in the mainline of my own project as of mbland/go-script-bash#165 (Travis CI results, Coveralls results).

If this PR contains too many changes at once, I'm happy to break it up. Also, this PR addresses sstephenson/bats#98, though I hadn't looked at sstephenson/bats#122 before.

macOS fix

The first commit in this set fixes the three existing tests that failed for Bash 3.2.57(1)-release, the default version for macOS 10.12.3 and other earlier versions of macOS and OS X. This was due to the ERR trap not always firing as it should; adding some guard logic to bats_error_trap before calling it from bats_teardown_trap resolved the issue.

Performance impact on Bats's own test suite

The remaining commits methodically eliminate subshells to improve overall test performance, especially those spawned by bats_capture_stack_trace via bats_debug_trap.

Under Bash 3.2.57(1)-release on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, the timing for the original set of tests (which don't include the four test name-related cases added by one of the later commits in this series) run via bin/bats test/ went from:

  44 tests, 0 failures

  real    0m7.565s
  user    0m3.664s
  sys     0m3.368s

to:

  real    0m4.319s
  user    0m2.559s
  sys     0m1.454s

Performance impact on a large Bats test suite

In my https://github.com/mbland/go-script-bash project, I'd already optimized the tests to disable Bats stack trace collection almost as much as possible, bringing the time for the full ./go test suite on the same machine, OS, and Bash version from O(7-8min) down to the following at mbland/go-script-bash@69ad67f:

  789 tests, 0 failures, 2 skipped

  real    2m57.196s
  user    1m30.371s
  sys     1m17.703s

Running again at the same commit, but using Bats with these changes included, the suite now runs in:

  real    1m24.217s
  user    1m1.195s
  sys     0m17.700s

Performance impact on Windows

The impact is even more dramatic on Windows. On the same MacBook Pro, running Windows 10 under VMware Fusion 8.5.3 with 4GB RAM, and the timing for ./go test before my changes to go-script-bash was in the O(50-60min) range for every version of Bash I have installed. Afterwards, it was down in the O(20+min range).

Using Bats with these changes included, running the MSYS2-based Bash 4.3.46(2)-release that ships with (what is probably a slightly outdated version of) Git for Windows, the suite now runs in:

  real    6m20.981s
  user    1m53.852s
  sys     3m16.015s

Running Bash 4.4.11(2)-release running under Cygwin:

  real    5m22.506s
  user    1m34.462s
  sys     2m41.568s

And running Bash 4.3.11(1)-release as part of the Windows Subsystem for Linux (i.e. Ubuntu on Windows 10):

  real    3m40.502s
  user    0m36.531s
  sys     3m1.516s

Other versions tested

In addition to the Bash versions mentioned above, I've also tested these changes using the following Bash versions:

4.2.25(1)-release: the version from the default Ubuntu 12.04.5/Precise image on Travis CI
4.3.42(1)-release: the latest on Alpine Linux
4.3.46(1)-release: the latest on Ubuntu 16.10
4.4.12(0)-release: the version from FreeBSD 10.3-RELEASE-p11
4.4.12(1)-release: the latest at the time of writing, on both Arch Linux and macOS via Homebrew

All of the go-script-bash tests passed on these other platforms in less than half the time from before. All of the Bats tests passed on every platform with the exception of a few on Alpine Linux; I can perhaps address these failures in the future.

Future steps

There are other subshells that can likely be eliminated in other parts of Bats, particularly those that happen at startup when bin/bats is collecting information on the test suite, which introduces a bit of a startup pause proportional to the number of tests to run (especially noticeable on Windows). However, this seems like a good place to stop for now.

mbland · 2017-09-27T10:37:02Z

Some notes on the Windows build:

Under Git for Windows Bash and plain MSYS2, the bin/bash symlink is translated to a file containing only the line:

../libexec/bats

And since these systems depend upon the first line starting with #! to tell whether it's executable, the tests must be run directly via libexec/bats test on those shells.

Currently, these tests are already failing in master under Git for Windows Bash, plain MSYS2, and Cygwin, and continue to fail with these changes:

 ✗ summary passing tests
   (in test file test/bats.bats, line 46)
     `[ "${lines[1]}" = "1 test, 0 failures" ]' failed
 ✗ summary passing and skipping tests
   (in test file test/bats.bats, line 52)
     `[ "${lines[2]}" = "2 tests, 0 failures, 1 skipped" ]' failed
 ✗ summary passing and failing tests
   (in test file test/bats.bats, line 58)
     `[ "${lines[4]}" = "2 tests, 1 failure" ]' failed
 ✗ summary passing, failing and skipping tests
   (in test file test/bats.bats, line 64)
     `[ "${lines[5]}" = "3 tests, 1 failure, 1 skipped" ]' failed

This one also fails under Git for Windows Bash:

✗ single-line tests
   (in test file test/bats.bats, line 254)
     `[ "${lines[3]}" =  'ok 3 input redirection' ]' failed

None of these fail under Windows Subsystem for Linux.

I'm happy to try to address these breakages before merging these changes, but I'm more happy to take them on as my next task following the merge of these changes. (And my mbland/go-script-bash test suite bangs on Bats pretty hard on Windows with no issues.)

mbland · 2017-09-28T10:42:24Z

Took a minute to look at the Windows failures this morning; went ahead and fixed them. First, I was mistaken about the single-line tests failure. It was failing on plain MSYS2, and that was because I didn't have diffutils installed. D'oh! (I thought about putting a command -v 'diff' >/dev/null || skip 'diff not installed' clause in there, but decided against it for now, because what civilized developer wouldn't install diff at some point?)

The other failures were trivial: paths containing the $FIXTURE_ROOT variable weren't quoted, and my Windows username containing a space broke them. Quoting the paths fixed the problem.

Those AppVeyor failures appear to be due to a missing config file. I can look into those tomorrow if someone doesn't beat me to it.

btamayo · 2017-09-28T16:49:31Z

Great! Thanks so much for doing that. Yes, I haven't set up the config file yet — should have mentioned that. I planed to do that today but if it doesn't happen you can also look into it tomorrow as you said :)

On Thu, Sep 28, 2017, 3:42 AM Mike Bland ***@***.***> wrote: Took a minute to look at the Windows failures this morning; went ahead and fixed them. First, I was mistaken about the single-line tests failure. It was failing on plain MSYS2, and that was because I didn't have diffutils installed. D'oh! (I thought about putting a command -v 'diff' >/dev/null || skip 'diff not installed' clause in there, but decided against it for now, because what civilized developer wouldn't install diff at some point?) The other failures were trivial: paths containing the $FIXTURE_ROOT variable weren't quoted, and my Windows username containing a space broke them. Quoting the paths fixed the problem. Those AppVeyor failures appear to be due to a missing config file. I can look into those tomorrow if someone doesn't beat me to it. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACNqJH4pj57_Nu1Ktr9tK4UUMPV1UZ3kks5sm3gRgaJpZM4Pliwu> .

-- Bianca https://biancatamayo.me

mbland · 2017-09-30T15:04:56Z

@btamayo I've pushed a .appveyor.yml and all is well. If you'd prefer, I'm happy to add it in a new PR first; this may also provide a way to compare the performance before this PR and after.

This follows the example from https://www.appveyor.com/docs/lang/ruby/ except that it doesn't need `install` or `before_test` steps. Inspired by bats-core/bats-core#8.

The big thing I did in #8 was to eliminate as many subshells, pipelines, and subprocesses as possible. Even though we normally rely write scripts because of these features, in a framework like Bats, they add up quickly and can have a big performance impact—especially on Windows, especially when running virtualized. Conseqently, replacing the previous `grep` pipeline with a `while` loop causes the suite to run ~9% faster on macOS, and ~19% faster on Windows (running under VMWare). The following times were collected by running `time bin/bats test` on a: MacBook Pro Processor: 2.9 GHz Intel Core i5 RAM: 8 GB 1867 MHz DDR3 Bash 4.4.12(1)-release running on macOS 10.12.6 ----------------------------------------------- Before this change: 47 tests, 0 failures real 0m4.703s user 0m2.712s sys 0m1.570s After this change: 47 tests, 0 failures real 0m4.312s user 0m2.369s sys 0m1.166s Git for Windows Bash 4.4.12(1)-release running on Windows 10 under VMWare Fusion 8.5.8 ------------------------------------------------------------------ Before this change: 47 tests, 0 failures real 0m19.832s user 0m4.170s sys 0m8.893s After this change: 47 tests, 0 failures real 0m16.675s user 0m3.463s sys 0m7.297s

btamayo · 2017-09-30T18:38:53Z

@mbland Yes please, that would be fantastic. Also, just to be sure: Would our min bash version be 3.2 for macOS then, correct? Let me know if you think that's reasonable and I can add it to the README (not sure if its there yet).

mbland · 2017-09-30T19:06:51Z

@btamayo Just filed #10 with the CI config changes and a bugfix for macOS. And yes, the min version would be 3.2.57(1)-release for macOS.

Preserves existing behavior. Next step will be to take the target variable name as the second argument.

This is part of the effort to improve performance by reducing the number of command substitutions/subshells spawned by `bats_debug_trap`. Under Bash 3.2.57(1)-release on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, this makes `bin/bats test/` go from: 44 tests, 0 failures real 0m7.565s user 0m3.664s sys 0m3.368s to: real 0m6.449s user 0m3.290s sys 0m2.665s

Also eliminates a subshell.

Also replaces `sed` invocation with a `while` loop, saving a subprocess.

Also, `bats-preprocess` now converts DOS/Windows CRLF line endings.

Somehow this is ever-so-slightly faster.

This is part of the effort to improve performance by reducing the number of command substitutions/subshells spawned by `bats_debug_trap`. Under Bash 3.2.57(1)-release on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, this makes `bin/bats test/` go from the following for the previous commit: 44 tests, 0 failures real 0m5.293s user 0m2.853s sys 0m2.087s to: real 0m4.319s user 0m2.559s sys 0m1.454s

This is in anticipation of refactoring away the `$(eval echo "$quoted_name")` command substitution.

This is part of the effort to improve performance by reducing the number of command substitutions/subshells. Under Bash 3.2.57(1)-release on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, this shaves off O(0.15s) from the test suite at the previous commit, but I anticipate this effect being magnified on Windows platforms.

Under Bash 3.2.57(1)-release and 4.4.12(1)-release on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, this shaves off O(0.4s) from the current test suite. Before the change: 46 tests, 0 failures real 0m4.392s user 0m2.489s sys 0m1.467s After the change: real 0m3.980s user 0m2.312s sys 0m1.233s

Under Bash 3.2.57(1)-release and 4.4.12(1)-release on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, this shaves off O(0.1s) from the current test suite. Before the change: 46 tests, 0 failures real 0m3.983s user 0m2.320s sys 0m1.241s After the change: real 0m3.861s user 0m2.276s sys 0m1.174s

Under Bash 3.2.57(1)-release and 4.4.12(1)-release on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, this shaves off O(0.25s) from the current test suite. Before the change: 46 tests, 0 failures real 0m3.851s user 0m2.273s sys 0m1.166s After the change: real 0m3.595s user 0m2.171s sys 0m1.048s

Under Bash 3.2.57(1)-release and 4.4.12(1)-release on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, this shaves off O(0.04-0.05s) from the current test suite. Very minor, but it's a straightforward change that may provide a minor-yet-noticeable effect on Windows. Before this change: 46 tests, 0 failures real 0m3.588s user 0m2.171s sys 0m1.046s After this change: real 0m3.538s user 0m2.119s sys 0m0.941s

Under Bash 3.2.57(1)-release and 4.4.12(1)-release on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, this shaves off O(0.16s) from the current test suite. Before this change: 46 tests, 0 failures real 0m3.541s user 0m2.125s sys 0m0.937s After this change: real 0m3.372s user 0m2.031s sys 0m0.894s

While the performance impact of these changes are in the noise under macOS 10.12.3 on a on a MacBook Pro with a 2.9GHz Intel Core i5 CPU and 8GB 1867MHz DDR3 RAM, eliminating these subshells makes the code more consistent. I did try removing `buffer` to eliminate yet more subshells, but the flickering of the output did prove annoying, so I'm not removing it.

The four test cases updated in this commit were failing on my Windows virtual machine because my username contains a space. Quoting the file paths containing "$FIXTURE_ROOT" solved the problem.

mbland · 2017-09-30T19:14:12Z

Just rebased this on master after merging #10. @btamayo PTAL and approve, barring any concerns?

The big thing I did in #8 was to eliminate as many subshells, pipelines, and subprocesses as possible. Even though we normally rely write scripts because of these features, in a framework like Bats, they add up quickly and can have a big performance impact—especially on Windows, especially when running virtualized. Conseqently, replacing the previous `grep` pipeline with a `while` loop causes the suite to run ~9% faster on macOS, and ~19% faster on Windows (running under VMWare). The following times were collected by running `time bin/bats test` on a: MacBook Pro Processor: 2.9 GHz Intel Core i5 RAM: 8 GB 1867 MHz DDR3 Bash 4.4.12(1)-release running on macOS 10.12.6 ----------------------------------------------- Before this change: 47 tests, 0 failures real 0m4.703s user 0m2.712s sys 0m1.570s After this change: 47 tests, 0 failures real 0m4.312s user 0m2.369s sys 0m1.166s Git for Windows Bash 4.4.12(1)-release running on Windows 10 under VMWare Fusion 8.5.8 ------------------------------------------------------------------ Before this change: 47 tests, 0 failures real 0m19.832s user 0m4.170s sys 0m8.893s After this change: 47 tests, 0 failures real 0m16.675s user 0m3.463s sys 0m7.297s

btamayo

LGTM! Thanks for this. Might also be useful to add shellcheck at a later time in CI for others :)

The big thing I did in #8 was to eliminate as many subshells, pipelines, and subprocesses as possible. Even though we normally rely write scripts because of these features, in a framework like Bats, they add up quickly and can have a big performance impact—especially on Windows, especially when running virtualized. Conseqently, replacing the previous `grep` pipeline with a `while` loop causes the suite to run ~9% faster on macOS, and ~19% faster on Windows (running under VMWare). The following times were collected by running `time bin/bats test` on a: MacBook Pro Processor: 2.9 GHz Intel Core i5 RAM: 8 GB 1867 MHz DDR3 Bash 4.4.12(1)-release running on macOS 10.12.6 ----------------------------------------------- Before this change: 47 tests, 0 failures real 0m4.703s user 0m2.712s sys 0m1.570s After this change: 47 tests, 0 failures real 0m4.312s user 0m2.369s sys 0m1.166s Git for Windows Bash 4.4.12(1)-release running on Windows 10 under VMWare Fusion 8.5.8 ------------------------------------------------------------------ Before this change: 47 tests, 0 failures real 0m19.832s user 0m4.170s sys 0m8.893s After this change: 47 tests, 0 failures real 0m16.675s user 0m3.463s sys 0m7.297s

jasonkarns · 2017-10-02T17:07:57Z

libexec/bats

+      BATS_READLINK='true'
+    fi
+  fi
+  "$BATS_READLINK" "$1" || return 0


What's the rationale for the true fallback? (and for returning successfully if the command fails)

Would we not want to fail if readlink is not successful? Or at least just return the original argument?

The original code would have simply failed if neither (g)readlink were available (or return the actual error code if the command failed: ie, if the argument were not a link). Should we not keep this behavior?

resolve_link was originally called like this:

path="$(resolve_link "$name" || true)"

Without the || true the script would exit when the target wasn't a valid symlink; the new code just pushes the "always true" result into the function itself. As before, the loop as a whole exits when path becomes the empty string.

jasonkarns · 2017-10-02T17:14:49Z

libexec/bats

 resolve_link() {
-  $(type -p greadlink readlink | head -1) "$1"
+  if [[ -z "$BATS_READLINK" ]]; then


Is there a compelling reason to use the [[ bash extension here? I believe we're attempting to be POSIX compliant with bats, yes? So that bats can be used on minimal POSIX CI containers and whatnot?

(I realize that there are already instances of [[ in the source, but was thinking we could gradually move away from them to be POSIX compliant?)

I'm not aware of any directive to remain strictly POSIX compliant in Bats. If I'm mistaken, I'm happy to look into changing it; but [[ ]] is a bit more powerful. (At any rate, $() for command substitutions isn't POSIX compliant, as far as I can tell.)

The big thing I did in #8 was to eliminate as many subshells, pipelines, and subprocesses as possible. Even though we normally rely write scripts because of these features, in a framework like Bats, they add up quickly and can have a big performance impact—especially on Windows, especially when running virtualized. Conseqently, replacing the previous `grep` pipeline with a `while` loop causes the suite to run ~9% faster on macOS, and ~19% faster on Windows (running under VMWare). The following times were collected by running `time bin/bats test` on a: MacBook Pro Processor: 2.9 GHz Intel Core i5 RAM: 8 GB 1867 MHz DDR3 Bash 4.4.12(1)-release running on macOS 10.12.6 ----------------------------------------------- Before this change: 47 tests, 0 failures real 0m4.703s user 0m2.712s sys 0m1.570s After this change: 47 tests, 0 failures real 0m4.312s user 0m2.369s sys 0m1.166s Git for Windows Bash 4.4.12(1)-release running on Windows 10 under VMWare Fusion 8.5.8 ------------------------------------------------------------------ Before this change: 47 tests, 0 failures real 0m19.832s user 0m4.170s sys 0m8.893s After this change: 47 tests, 0 failures real 0m16.675s user 0m3.463s sys 0m7.297s

mbland added the Type: Enhancement label Sep 27, 2017

mbland self-assigned this Sep 27, 2017

mbland requested a review from btamayo September 27, 2017 10:22

mbland added a commit to mbland/go-script-bash that referenced this pull request Sep 30, 2017

Add .appveyor.yml

c97855a

This follows the example from https://www.appveyor.com/docs/lang/ruby/ except that it doesn't need `install` or `before_test` steps. Inspired by bats-core/bats-core#8.

mbland mentioned this pull request Sep 30, 2017

Add .appveyor.yml mbland/go-script-bash#219

Merged

mbland mentioned this pull request Sep 30, 2017

Initial code and test for duplicate test name warning. #9

Merged

mbland mentioned this pull request Sep 30, 2017

Add Appveyor configuration, update Travis CI configuration, fix macOS bug #10

Merged

mbland added 16 commits September 30, 2017 15:12

exec-test: Refactor bats_frame_* functions

a3396cb

Preserves existing behavior. Next step will be to take the target variable name as the second argument.

exec-test: Replace dirname call with %/*

39440c5

exec-test: Use printf -v in bats_trim_filename

eaa151f

exec-test: Use printf -v in bats_strip_string

3bc0379

exec-test: Replace type -t with command -F

698fa76

Also eliminates a subshell.

exec-test: Use printf -v in bats_extract_line

d294689

Also replaces `sed` invocation with a `while` loop, saving a subprocess.

exec-test: Invoke bats-preprocess directly

cf9a3b8

Also, `bats-preprocess` now converts DOS/Windows CRLF line endings.

preprocess: Use printf -v in encode_name

741c414

exec-test: Replace || { } with if [[ ]]; then

e613b31

Somehow this is ever-so-slightly faster.

preprocess: Add tests for vars, quotes in names

3ab495f

This is in anticipation of refactoring away the `$(eval echo "$quoted_name")` command substitution.

mbland added 4 commits September 30, 2017 15:12

Fix test failures due to unquoted variables

ebb192e

The four test cases updated in this commit were failing on my Windows virtual machine because my username contains a space. Quoting the file paths containing "$FIXTURE_ROOT" solved the problem.

mbland force-pushed the mbland-optimized branch from 4e4dd77 to ebb192e Compare September 30, 2017 19:13

btamayo approved these changes Sep 30, 2017

View reviewed changes

mbland merged commit 8538868 into master Sep 30, 2017

mbland deleted the mbland-optimized branch September 30, 2017 19:46

This was referenced Sep 30, 2017

Bump to v0.4.1? #16

Closed

Transfer bats#163 - Avoid undocumented usage of the head utility. #17

Merged

jasonkarns reviewed Oct 2, 2017

View reviewed changes

This was referenced Oct 3, 2017

1.0 bug fixes #20

Closed

Transfer bats#142 - This commit corrects the problem of incorrect TAP format for skipped tests #19

Merged

This was referenced Oct 8, 2017

Update _GO_BATS_URL and _GO_BATS_VERSION to bats-core/bats-core mbland/go-script-bash#223

Open

Deprecating the focus on posix sh compatibility #28

Closed

mbland mentioned this pull request Dec 4, 2017

Adds 'include' command to include another bats file #42

Closed

mbland mentioned this pull request Jun 3, 2018

Adds next version changelog #66

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix macOS/Bash 3.2 breakage; eliminate subshells from exec-test, preprocess #8

Fix macOS/Bash 3.2 breakage; eliminate subshells from exec-test, preprocess #8

Uh oh!

mbland commented Sep 27, 2017

Uh oh!

mbland commented Sep 27, 2017

Uh oh!

mbland commented Sep 28, 2017

Uh oh!

btamayo commented Sep 28, 2017 via email

Uh oh!

mbland commented Sep 30, 2017

Uh oh!

btamayo commented Sep 30, 2017

Uh oh!

mbland commented Sep 30, 2017

Uh oh!

mbland commented Sep 30, 2017

Uh oh!

btamayo left a comment

Uh oh!

jasonkarns Oct 2, 2017

Uh oh!

mbland Oct 2, 2017

Uh oh!

jasonkarns Oct 2, 2017 •

edited

Loading

Uh oh!

mbland Oct 2, 2017

Uh oh!

Uh oh!

Fix macOS/Bash 3.2 breakage; eliminate subshells from exec-test, preprocess #8

Fix macOS/Bash 3.2 breakage; eliminate subshells from exec-test, preprocess #8

Uh oh!

Conversation

mbland commented Sep 27, 2017

macOS fix

Performance impact on Bats's own test suite

Performance impact on a large Bats test suite

Performance impact on Windows

Other versions tested

Future steps

Uh oh!

mbland commented Sep 27, 2017

Uh oh!

mbland commented Sep 28, 2017

Uh oh!

btamayo commented Sep 28, 2017 via email

Uh oh!

mbland commented Sep 30, 2017

Uh oh!

btamayo commented Sep 30, 2017

Uh oh!

mbland commented Sep 30, 2017

Uh oh!

mbland commented Sep 30, 2017

Uh oh!

btamayo left a comment

Choose a reason for hiding this comment

Uh oh!

jasonkarns Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

mbland Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

jasonkarns Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbland Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasonkarns Oct 2, 2017 •

edited

Loading