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

mbland commented 7 years ago

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 https://github.com/mbland/go-script-bash/pull/165 (Travis CI results, Coveralls results).

If this PR contains too many changes at once, I'm happy to break it up. Also, I just noticed that this PR addresses #98, without even having looked at #122 before.

cc: @ztombol @stroupaloop

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 https://github.com/mbland/go-script-bash/commit/69ad67f8474522540c9f90d7fd4f93903aaa63ce:

  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.

ret2libc commented 7 years ago

AMAZING!

I also noticed all those time spent in the debug handler and was trying to do something about that, but... These commits are great!

Unfortunately it seems you won't have your commits merged... :(

Sylvain303 commented 7 years ago

@ret2libc, fortunately there will be soon a new Maintainer(s) #150

You may know, how to merge the PR in your own fork?

http://stackoverflow.com/questions/6022302/how-to-apply-unmerged-upstream-pull-requests-from-other-forks-into-my-fork

mbland commented 7 years ago

@Sylvain303 Is there any news on #150 that hasn't been posted to that thread? There hasn't been any response to it since I threw my hat in the ring as a potential maintainer.

@ret2libc I've since done a few more optimizations, posted to my fork as the mbland/bats optimized-20170317 tag. It's effectively "done" in terms of optimizing by eliminating subshells and subprocesses. No improvements as dramatic as in this PR, but still substantial. I've debated posting them to this PR, or opening a new PR, but decided to wait until this one and #150 finally get traction.

ret2libc commented 7 years ago

@Sylvain303 thanks I'm already aware of that ;)

@mbland that's really cool, thanks. I hope this thing will get some attention soon.

mbland commented 7 years ago

Oh, and for anyone who isn't familiar with managing repos with multiple remotes, the Git commands to pull my changes into your repo should be something like:

$ git remote add mbland git@github.com:mbland/bats.git
$ git fetch mbland --tags
$ git checkout -b optimized optimized-20170317

To see the tag comments with the summaries (whereby optimized-20170205 corresponds to the branch used for #210):

$ git show optimized-20170205
$ git show optimized-20170317

Sylvain303 commented 7 years ago

@mbland, no, no special news for #150, I'm just optimist. :)

Coming back to your PR, here:

https://github.com/sstephenson/bats/pull/210/files#diff-669b297cddac27dc2ecc950eb92aa5fcL316

Are changes equivalent? Previous version does deleting \r, does your version do the same?

I'm trying to merge your PR in my internal unittest. I'm using a "python trick like" sourcing detection:

#!/bin/bash
main() {
  echo "sourced=$sourced"
}

[[ $0 != "$BASH_SOURCE" ]] && sourced=1 || sourced=0

if [[ $sourced -eq 0  ]] ; then
   main "$@"
fi

My current bats-preprocess version (with your modifications).

As you changed in bats-exec-test, the internal behavior, I'm asking if it benefits for something here? I mean sourcing it instead of executing it.

a sub-shell is spawned for:

$(< "$BATS_TEST_FILENAME")

It would be easier for me to keep source detection feature, than having to rely on some other trick. More on bats-preprocess I feel it should perform input cleanup itself, but that may be another question/optimization.

Could you also enable issues, on your bats fork, so I can comment there?

Sylvain303 commented 7 years ago

@mbland, I found where you've handled '\r' dos end-of-line:

https://github.com/sstephenson/bats/pull/210/files#diff-e5a78c356c58edaf23c12ce3dae62023R39

great!

sstephenson / bats