New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@mbland

mbland commented Feb 15, 2017

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, 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 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 added some commits Feb 15, 2017

exec-test: Work around Bash 3.2.57 ERR trap bug
When running under Bash 3.2.57(1)-release on macOS, the following tests
would fail because `BATS_ERROR_STACK_TRACE` would be empty, and hence no
information about the actual error would get printed:

- one failing test
- failing test with significant status
- failing test file outside of BATS_CWD

This is because each of these cases use `FIXTURE_ROOT/failing.bats`, and
the `ERR` trap would not fire for its `eval "( exit ${STATUS:-1} )"`
line. Changing it to `exit ${STATUS:-1}` produced the same effect, and
changing it to `return ${STATUS:-1}` would cause the output to point to
the previous line, which executes `true`.

However, the correct status would be reported to the `EXIT` trap, so now
we call `bats_error_trap` at the very beginning of `bats_teardown_trap`.

All the existing tests now pass under Bash 3.2.57(1)-release, Bash
4.2.25(1)-release (the version from the default Ubuntu 12.04.5/Precise
image on Travis CI), and Bash 4.4.12(1)-release.
exec-test: Refactor bats_frame_* functions
Preserves existing behavior. Next step will be to take the target
variable name as the second argument.
exec-test: Use printf -v in bats_frame_* functions
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
exec-test: Replace `type -t` with `command -F`
Also eliminates a subshell.
exec-test: Use `printf -v` in bats_extract_line
Also replaces `sed` invocation with a `while` loop, saving a subprocess.
exec-test: Invoke bats-preprocess directly
Also, `bats-preprocess` now converts DOS/Windows CRLF line endings.
exec-test: Replace `|| { }` with `if [[ ]]; then`
Somehow this is ever-so-slightly faster.
exec-test: Replace `caller` with `FUNCNAME`, etc.
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
preprocess: Eliminate eval in subshell
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.
preprocess: Add tests for vars, quotes in names
This is in anticipation of refactoring away the `$(eval echo
"$quoted_name")` command substitution.
@ret2libc

This comment has been minimized.

Show comment
Hide comment
@ret2libc

ret2libc Mar 24, 2017

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... :(

ret2libc commented Mar 24, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Sylvain303

Sylvain303 Mar 24, 2017

Contributor

@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

Contributor

Sylvain303 commented Mar 24, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mbland

mbland Mar 24, 2017

@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.

mbland commented Mar 24, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ret2libc

ret2libc Mar 24, 2017

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

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

ret2libc commented Mar 24, 2017

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

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

@mbland

This comment has been minimized.

Show comment
Hide comment
@mbland

mbland Mar 24, 2017

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

mbland commented Mar 24, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Sylvain303

Sylvain303 Mar 27, 2017

Contributor

@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?

Contributor

Sylvain303 commented Mar 27, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Sylvain303
Contributor

Sylvain303 commented Mar 29, 2017

body="${BASH_REMATCH[2]}"
name="$(eval echo "$quoted_name")"

This comment has been minimized.

@Sylvain303

Sylvain303 Mar 29, 2017

Contributor

doesn't removing eval, line 41 on $quoted_name, remove expected behavior on variable expansion in the string or such?

@Sylvain303

Sylvain303 Mar 29, 2017

Contributor

doesn't removing eval, line 41 on $quoted_name, remove expected behavior on variable expansion in the string or such?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment