Improve performance of bisect using new forking bisect runner

[myronmarston]

Until now, --bisect has shelled out to run each subset. I've realized this is pretty inefficient: you keep paying the cost to boot RSpec and the application environment over and over again. I thought we could perhaps improve performance by running the specs using fork instead of shelling out. This PR is the result of my attempt.

Results

The improvement of the new forking bisect runner can make a big difference, but the amount of improvement depends a lot on an individual project's spec suite. Spec suites that boot quickly, and have a runtime dominated by the specs themselves, will see only marginal improvements. Any spec suite that has significant boot up time (e.g. to load rails or some other heavyweight dependency) should see a noticeable improvement.

I did some testing using the unit specs of plines, and old project I worked on years ago. It contains 343 unit specs which run pretty quickly (time bin/rspec spec/unit takes about 2 seconds). Running bin/rspec spec/unit --bisect using the existing shell runner takes 9.8 seconds. With the new fork runner, it drops to 6.3 seconds--about a 33% improvement.

However, if we simulate the typical boot time of a rails app by adding a sleep 5 to spec_helper, the difference is much more dramatic. With the shell runner, a bisect run takes 108.9 seconds. With the fork runner, it takes 11.7 seconds.

If you really care about speed, you can pass additional --require options (or even require a file that pre-loads all of your application) when bisecting to ensure as much is pre-loaded as possible.

Compatibility concerns

As nice as the performance improvements from the fork runner are, I don't think it's going to work for all spec suites. Here are some potential issues:

• Platforms that don't support fork. AFAIK, this is only windows, but we have to support it, obviously. We can keep using the shell runner for these environments. This PR does that by checking Process.respond_to?(:fork) before deciding which bisect runner to default to.
• Projects that use spring or some other forking runner. I don't know much about spring (or any of the other similar projects: zeus, spork, etc), but I understand it uses forking to work. I don't know if there will be any conflict, if RSpec uses forking internally for --bisect and spring also does.
• Projects that put one-time setup at the top level of spec_helper instead of in a before(:suite) hook. The forking runner loads --require files only once (which is a big part of where it gets its perf improvements!) but that means that spec suite bootstrapping logic written at the top level of spec_helper will only get executed once, and not once per spec run, as with the shell runner. Usually this isn't a big deal, but imagine a spec suite that booted selenium at the top level of spec_helper, and then shut down selenium in an after(:suite) hook. In such a case, selenium would get shutdown after the first spec run, and it would not be available for use in subsequent spec runs. With the shell runner, this isn't a problem, because it re-loads spec_helper each time.

Given these compatibility concerns, I'm not sure if we should default to the new :fork runner on platforms that support :fork or not. If we stick with :shell as the default and make users opt-in to :fork, most users will miss out on the performance improvement. But I also don't want to break bisect for some users. Then again, I spent a bunch of time trying to create a situation like that last case, and failed to actually trigger a problem. So maybe the forking runner isn't going to cause problems for any users.

What do others think?

[myronmarston]

This is green and ready for review, @rspec/rspec.

[JonRowe]

Looks good to me! My 2¢ on whether we should default to this is that we should, but if you're concerned we could do a deprecation style warning for not setting which one you're using and explain the choice?

[myronmarston]

Looks good to me!

What, no suggestions whatsoever? Is that a first for a PR of this size? 😄

but if you're concerned

I'm vaguely concerned--but mostly it's a concern about the fact that we don't yet know how frequently the fork runner will cause problems for users. I've never used spring (and don't have easy access to a project that does) but that's a big unknown given it's use of forking and the potential for them to step on each other's toes. If the forking runner is incompatible with spring, we probably don't want to default to the fork runner (or find a way to detect that rspec was already loaded in a pre-forking runner like spring--we could probably stash the original Process.pid in a global variable when lib/rspec/core.rb first loads, then check that later to see if we're in the same process when we start bisecting).

I'm also not sure how much of a problem the "setup logic in main scope of spec_helper vs in a before(:suite) hook" issues is...as I said, I tried to trigger that condition and failed. Logically, it's easy to understand how a problem could occur there but actually triggering it on purpose is trickier.

we could do a deprecation style warning for not setting which one you're using and explain the choice

We could go this route, but I'd really prefer not to. It's kinda the worst option in my mind. Setting the bisect runner is a little tricky compared to a normal config option. The standard way people did config before rspec --init generated --require spec_helper would not even work to allow you to set the bisect runner. Historically, many projects didn't put a --require option in .rspec, and instead manually required spec_helper from each spec file. Such an approach would not work to set the bisect runner, as --bisect does not load any spec files before instantiating the runner. The config option only works if it is set in a file loaded via a --require option.

After writing that up, I'm realizing it would be beneficial to users if we warned or errored when config.bisect_runner = runner_option has no effect due to it not being a --require file. Thoughts?

[JonRowe]

After writing that up, I'm realizing it would be beneficial to users if we warned or errored when config.bisect_runner = runner_option has no effect due to it not being a --require file. Thoughts?

That seems like a good idea!

[myronmarston]

OK, @JonRowe, I implemented the change we talked about and also added some more output when the :fork runner is used on a test suite that has problems with it. For future reference, here's some example spec_helper code that triggers this problem:

RSpec.configure do |config|
  File.write("/tmp/some_resource", "in_use")

  config.after(:suite) do
    File.exist?("/tmp/some_resource") && File.delete("/tmp/some_resource")
  end

  config.before do
    unless File.exist?("/tmp/some_resource")
      raise "file missing"
    end
  end
end

The fix for this problem is to use the :shell runner (as explained by the text I added) or move the File.write into a before(:suite) hook so it happens on each subset run (but that's way harder to explain so I didn't put that in the warning).

Thoughts?

[myronmarston]

@xaviershay -- you reviewed several of my prepatory PRs for this feature...do you care to review this before it gets merged as well?

[myronmarston]

Gonna merge this.

[JonRowe]

Sorry I've been trying to get around to reviewing this for days, luckily it all looks good :)