Stash methods that get and set thread local variables to allow the user to mock them

[manueljacob]

Fixes rspec/rspec#103.
Alternative to #581 to fix #580

[pirj]

Can you please add a spec that would fail before your changes?

[pirj]

@ioquatix we fixed a side issue of not being able to mock get_instance_variable. Does it still work for your uses?

[JonRowe]

Typically we've used a constant for these sort of stashing.

Suggested change

	@thread_variable_set = Thread.instance_method(:thread_variable_set)
	THREAD_SET = Thread.instance_method(:thread_variable_set)

[manueljacob]

Typically we've used a constant for these sort of stashing.

Done.

[pirj]

Can you please make sure you’ve pushed it?

[manueljacob]

Oops, I forgot to stage the changes before amending and pushing.

[JonRowe]

I would argue that you shouldn't really be mocking the internals of thread, its not something you own, and this implementation still wouldn't allow you to mock Thread.current which is more likely IMO that individual methods...

[ioquatix]

@pirj I stopped using RSpec so I don't have any strong opinion about it. However, I still think it would be better to use Thread.attr_accessor :rspec_state or something like that, would have also avoided this issue...

[manueljacob]

Fixes rspec/rspec#103. Alternative to #581 to fix #580

@pirj You added this in the PR description. But unless I’m missing something, this PR is neither an alternative to #581 nor could have fixed #580. It fixes an issue introduced by #581.

[manueljacob]

Can you please add a spec that would fail before your changes?

Done. Not sure how I missed that.

[ioquatix]

LGTM, however have you measured the performance overhead?

[manueljacob]

LGTM, however have you measured the performance overhead?

I pushed some performance optimization (using UnboundMethod#bind_call) instead of separate UnboundMethod#bind and Method#call, and did some quick tests. I replaced stdout and stderr to avoid writing 1000000 dots to the terminal.

require 'benchmark'
require 'rspec'

out = StringIO.new

puts Benchmark::CAPTION
puts(Benchmark.measure do
  RSpec.describe do
    1000000.times do
      it do
      end
    end
  end

  RSpec::Core::Runner.run([], out, out)
end)

out.seek(0)
puts out.readlines[-3..-2]

Before this PR:

      user     system      total        real
 85.016527   2.258209  87.274736 ( 87.424472)
Finished in 59.82 seconds (files took 27.66 seconds to load)
1000000 examples, 0 failures
ruby rspec_perf.rb  86,45s user 2,69s system 99% cpu 1:29,46 total

After this PR:

      user     system      total        real
 85.154865   2.142675  87.297540 ( 87.447185)
Finished in 59.88 seconds (files took 27.65 seconds to load)
1000000 examples, 0 failures
ruby rspec_perf.rb  86,53s user 2,60s system 99% cpu 1:29,47 total

From these results, it’s only very slightly slower. However, the test conditions were not perfect (light background activity, power saving enabled).

EDIT: These measurements were done on Ruby 3.3.1.

[manueljacob]

UnboundMethod#bind_call was added in Ruby 2.7, so I re-added the UnboundMethod#bind / Method#call code for older Ruby versions. On Ruby 2.6, I observed a much larger slowdown of up to 10% on the above benchmark. However, variance is high on my current benchmarking setup.

To do some proper benchmarking, I should follow that and that. However, currently I don’t have a suitable machine available for that.

[ioquatix]

Nice work, I assume pre-bind_call is more than trivially slower?

[manueljacob]

Nice work, I assume pre-bind_call is more than trivially slower?

The up to 10% slowdown on Ruby 2.6 described in my previous comment was this PR compared to the main branch. However, I’m not sure how much of the slowdown comes from the lack of bind_call vs. bind/call generally being less optimized on old Ruby versions. As I observed a lot a variance on the benchmark results especially on Ruby 2.6, I’ll be careful with conclusions.

I can do more benchmarks as soon as I can free some machine to do them without background load (which could be a few days). How detailed should it be? Does it even make sense to benchmark on end-of-life Ruby versions?

[ioquatix]

I would personally not implement a feature like this if the performance was significantly impacted.

There are many users of RSpec - probably millions of invocations per day. How many of them want even 1-2% slower execution because of this feature?

[JonRowe]

I'm closing this in favour of #610, I think in light of the conversation here, @ioquatix's original suggestion of patching in an accessor is warranted, especially because if people are mocking thread methods they are likely to mock Thread.current