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

Make memoized helpers threadsafe #1858

Merged
merged 1 commit into from Apr 3, 2015

Conversation

Projects
None yet
3 participants
@JoshCheek
Copy link
Contributor

JoshCheek commented Feb 2, 2015

Make memoized is a threadsafe object instead of a hash

Accordingly, rename RSpec::Core::MemoizedHelpers::ContextHookMemoizedHash -> ContextHookMemoized

Motivation

When working in a truly threaded environment (e.g. Rbx and JRuby),
you can write a test like the one below, which should pass.
But, it will fail sometimes, because two threads request the uninitialized counter
concurrently. The first one to receive the value will then be
incrementing a counter, that is later overwritten when the second
thread's let block returns.

You can verify this by running the code sample below in Rubinius.
After some number of attempts, the value will be less than 10k.
If you then make the let block a let! block, it will memoize
before the threads are created, and the values will be correct again.
While this is a reasonable solution, it is incredibly confusing
when it arises (I spent a lot of hours trying to find the bug in my code),
and requires that the user understand it's something they need to
be aware of and guard against.

class Counter
  def initialize
    @mutex = Mutex.new
    @count = 0
  end

  attr_reader :count

  def increment
    @mutex.synchronize { @count += 1 }
  end
end

RSpec.describe Counter do
  let(:counter) { Counter.new }

  it 'increments the count in a threadsafe manner' do
    threads = 10.times.map do
      Thread.new { 1000.times { counter.increment } }
    end
    threads.each &:join
    expect(counter.count).to eq 10_000
  end
end

Decisions

Create an object because only #fetch was being called on the hash,
and to make that threadsafe required adding a new method.
At that point, there is no value in having it subclass Hash,
having to support an entire API that nothing else uses.
So just make it an object which wraps a hash and presents a memoized
getter/setter. Also named the method in such a way as to make it
very clear that it's not a hash, so a dev won't mistakenly think
they are working with one when they aren't.

I had initially tried writing the test with Fibers. It was much shorter.
But Fibers don't work, because they execute within the same thread,
so you can't identify the difference between another thread setting
the memoized value that we need to respect, and a call to super()
setting the value, which we need to override.
(eg

subject { super() + [:override] }
)

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 2, 2015

Thanks for looking into this. FWIW, we've explicitly documented let as being non-threadsafe for years:

https://github.com/rspec/rspec-core/blob/v3.1.7/lib/rspec/core/memoized_helpers.rb#L209-L211

...which dates from 78eb9e8, nearly 3 years ago.

(Although, reading that now, it's perhaps not as explicit as it should be).

I'm a bit concerned with the additional overhead the synchronization adds. Before proceeding, I'd want to see a benchmark showing how perf is affected by this. Given that you're the first user to ever report a threadsafety problem, and how common let is used in RSpec suites, if this had a large effect on perf, I'd lean towards improving our documentation to make it even more explicit that let is non-threadsafe, and encouraging users to use let! (the eager, non-lazy version of let) for cases where they are spawning threads instead.

Can you work up a benchmark? We have a directory for benchmarks. For things like this that can't be easily isolated into alternate versions of a single method, we've had success using method aliasing to toggle between alternate implementations. Here's one example, from #1812, that may help make it more clear what I mean. Alternately, you could look into trying readygo for a benchmark since it appears to support comparing implementations from multiple git branches. I've been meaning to try out readygo but haven't had a chance to yet. If you want help with the benchmark (there are a few gotchas we've run into when benchmarking RSpec stuff in the past...) let us know.

Thanks!

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 2, 2015

Thinking about this some more...if the benchmarks show a significant perf difference and we can't find a way to get that down, we could potentially make threadsafe let available as an opt-in config option, where the config option would replace the simpler, non-threadsafe implementation we have now with your more robust implementation.

@JonRowe

This comment has been minimized.

Copy link
Member

JonRowe commented Feb 2, 2015

.if the benchmarks show a significant perf difference and we can't find a way to get that down, we could potentially make threadsafe let available as an opt-in config option

👍 :)

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Feb 2, 2015

Thanks for looking into this. FWIW, we've explicitly documented let as being non-threadsafe for years:

https://github.com/rspec/rspec-core/blob/v3.1.7/lib/rspec/core/memoized_helpers.rb#L209-L211

...which dates from 78eb9e8, nearly 3 years ago.

(Although, reading that now, it's perhaps not as explicit as it should be).

Yeah, it's one of those things where I've been using it so long, I can't even remember when the last time I read the docs would have been. I wouldn't have noticed that back then, my approach was to simplify my world by ignoring the existence of threads :P

I'm a bit concerned with the additional overhead the synchronization adds. Before proceeding, I'd want to see a benchmark showing how perf is affected by this.

Sure. Any real suite I know of would be a bad choice b/c the majority of their time would be dominated by IO. So, I made a separate suite where I just generate craploads of specs and lets.

https://github.com/JoshCheek/benchmark-rspec-core-threadsafe-let

Stats:

Attribute Value
Depth 7
Spec file size 4.4M
Number of lines in spec file 123,301
Number of examples 13,700
Number of let statements 82,203
Number of calls to super 68,502
Average time for 10 runs on master 2.6s
Average time for 10 runs on this branch 3.2s
Average difference 0.6s

Personally I don't know of any suites this large, and they would probably include assertions, as well :) This is specifically generated to hit its worst case scenarios (up to 7 levels of super).

In general, Mutext lock/unlock should be fast https://gist.github.com/hellerbarde/2843375#file-latency_humanized-markdown so the overhead is probably mostly the object allocation. Previously there would have been 1 hash per example, this would add a Memoized and a Mutex, and an Array for each memoized value. So that should be light, even a big project shouldn't have enough examples for you to ever notice. One of the reasons I chose the literal was just to reduce this cost, though I doubt it would ever be noticed.

Given that you're the first user to ever report a threadsafety problem, and how common let is used in RSpec suites, if this had a large effect on perf, I'd lean towards improving our documentation to make it even more explicit that let is non-threadsafe, and encouraging users to use let! (the eager, non-lazy version of let) for cases where they are spawning threads instead.

I'd expect the GIL saves a lot of people, but I dislike the idea of making it harder to get off MRI, because programs suddenly fail in difficult to diagnose ways, which turn out to not actually be bugs in the user's code, but in their use of the test suite.

Here's one example, from #1812, that may help make it more clear what I mean. Alternately, you could look into trying readygo for a benchmark since it appears to support comparing implementations from multiple git branches. I've been meaning to try out readygo but haven't had a chance to yet. If you want help with the benchmark (there are a few gotchas we've run into when benchmarking RSpec stuff in the past...) let us know.

Aah, ran out the door too quick this morning and didn't read the last bit. If the one I've got above isn't what you're looking for, let me know and I'll figure out how to work with this suite.

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 3, 2015

Aah, ran out the door too quick this morning and didn't read the last bit. If the one I've got above isn't what you're looking for, let me know and I'll figure out how to work with this suite.

Yeah, that's not quite what I'm looking for. Ideally, here's what I'd like to see from a benchmark:

  • Given let(:name) { nil } how many iterations per second are each of these things on master vs your branch:
    • Calling name once and clearing memoization (or instantiating a new object or whatever)
    • Calling name 10x and clearing memoization (or instantiating a new object or whtatever)
  • Those same two benchmarks for when the let uses super.

The hope here is to be able to see that "threadsafe let is x% slower than non-threadsafe let for case y", so that we can make an informed decision about this. The { nil } block is chosen to try to limit all computation to just the overhead of let itself. Given that let is defined in a module, you should be able to include that module in your own benchmarkable class so that we can cut out the overhead of the rest of RSpec for the purpose of these benchmarks. I think that having separate benchmarks for calling the let-defined method once vs 10x is useful, since in your implementation there is extra overhead the first time let is called and in many cases a let definition may only be called a single time from an example, so we care about the perf of that case. On the other end of the spectrum, some examples may call a let definition many times, and the 10x benchmarks will be representative of that case and give us an idea of the perf degradation.

Isolating one feature of RSpec for the purpose of a benchmark -- particularly when we're comparing multiple implementations -- is certainly not straightforward and easy, so please holler if you want any help.

JoshCheek added a commit to JoshCheek/rspec-core that referenced this pull request Feb 7, 2015

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Feb 7, 2015

Fixed the Travis failures and added benchmarks. Here are the benchmark results.

##############
#            #
#  versions  #
#            #
##############
RUBY_VERSION             2.2.0
ruby -v                  ruby 2.2.0p0 (2014-12-25 revision 49005) [x86_64-darwin13]
Benchmark::IPS::VERSION  2.1.1
rspec-core SHA           9daea3316b07736ac536f104d443d2f29e7ed722

##########################################
#                                        #
#  1 call to let -- each sets the value  #
#                                        #
##########################################
Calculating -------------------------------------
          threadsafe   120.000  i/100ms
      non-threadsafe   121.000  i/100ms
-------------------------------------------------
          threadsafe      7.949k (±19.7%) i/s -     35.760k
      non-threadsafe      8.541k (±21.7%) i/s -     35.695k

Comparison:
      non-threadsafe:     8541.2 i/s
          threadsafe:     7948.8 i/s - 1.07x slower

###################################################
#                                                 #
#  10 calls to let -- 9 will find memoized value  #
#                                                 #
###################################################
Calculating -------------------------------------
          threadsafe    45.000  i/100ms
      non-threadsafe    49.000  i/100ms
-------------------------------------------------
          threadsafe      4.894k (±18.7%) i/s -     20.835k
      non-threadsafe      5.603k (±17.8%) i/s -     23.128k

Comparison:
      non-threadsafe:     5603.0 i/s
          threadsafe:     4894.2 i/s - 1.14x slower

#######################################
#                                     #
#  1 call to let which invokes super  #
#                                     #
#######################################
Calculating -------------------------------------
          threadsafe    23.000  i/100ms
      non-threadsafe    25.000  i/100ms
-------------------------------------------------
          threadsafe      2.796k (±15.7%) i/s -     12.029k
      non-threadsafe      3.313k (±13.3%) i/s -     14.200k

Comparison:
      non-threadsafe:     3312.7 i/s
          threadsafe:     2795.8 i/s - 1.18x slower

#########################################
#                                       #
#  10 calls to let which invokes super  #
#                                       #
#########################################
Calculating -------------------------------------
          threadsafe    18.000  i/100ms
      non-threadsafe    18.000  i/100ms
-------------------------------------------------
          threadsafe      2.283k (±12.3%) i/s -      9.684k
      non-threadsafe      2.409k (±12.6%) i/s -     10.890k

Comparison:
      non-threadsafe:     2408.8 i/s
          threadsafe:     2283.1 i/s - 1.06x slower
#
# I ran these by adding "benchmark-ips" to ../Gemfile
# then exported BUNDLE_GEMFILE to point t it
# then ran `bundle exec rspec threadsafe_let_block.rb`

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

For future reference, the Gemfile supports the git-ignored Gemfile-custom specifically so contributors (and core devs) can add one-off gems that aren't generally used by the travis build or by most contributors (such as benchmark-ips):

eval File.read('Gemfile-custom') if File.exist?('Gemfile-custom')

So there's no need to edit Gemfile or export BUNDLE_GEMFILE the next time if you ever contribute this kind of thing again :).

This comment has been minimized.

@JoshCheek

JoshCheek Feb 7, 2015

Contributor

Nice 👍

Class === described ? described.new : described
end
end
end

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

Is the subject definition here (and below) actually needed by this benchmark? It looks to me like it's not used and extra unused method defs obscure what's actually needed here, IMO.

This comment has been minimized.

@JoshCheek

JoshCheek Feb 7, 2015

Contributor

That's true, this is the non threadsafe subject, but the tests just hit the let.

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

I also noticed that the benchmarks create example groups and examples, which is additional overhead outside what we are trying to benchmark here. I'm going to take a stab at revising this benchmark to deal with that. I'll push a branch and let you know when I have something.

puts "RUBY_VERSION #{RUBY_VERSION}"
puts "ruby -v #{`ruby -v`}"
puts "Benchmark::IPS::VERSION #{Benchmark::IPS::VERSION}"
puts "rspec-core SHA #{`git log --pretty=format:%H -1`}"

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

I had never thought about including the SHA in the benchmark output...cool idea! I'll probably steal this in the future for other benchmarks :).

end.run

expect(values.size).to eq 2
expect(values[0]).to eq values[1]

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

Can't these two lines be simplified to just one expectation?

expect(values).to eq([:value1, :value1])

That seems like a better, stronger expectation, anyway; it's important that its the first value that is returned by the let def, right?

This comment has been minimized.

@JoshCheek

JoshCheek Feb 7, 2015

Contributor

As of right now, no, b/c w/ current implementation, it could also be [:value2, :value2] I could maybe further synchronize it so that you know when the threads run in relation to each other, and then get this kind of confidence, but it could get pretty verbose without creating abstractions. Which I don't really mind doing, I had thought about it, even, but it just seemed heavy since it's just for this one test.


# provide the values they are blocking for
value_queue << :value1
value_queue << :value2

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

WDYT about using symbols that describe the roles these two play rather than the more generic :value1 and :value2? Something like:

value_queue << :value_let_should_return
value_queue << :value_let_should_not_return_if_threadsafe
@@ -1,3 +1,5 @@
require 'thread'

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

We generally try to load as little of the standard library as possible. Consider the case of a user developing a gem that uses multithreading, but they forget to require 'thread': their specs would still pass (since RSpec itself is loading 'thread'), but when they release the gem users of the gem would be likely to hit errors unless something else in their environment was loading 'thread'. Loading less of the stdlib also helps keep the boot time of RSpec itself down (although any individual require is unlikely to be noticable, of course). Ultimately, we don't have an absolute rule about this--we load optparse for CLI option parsing since it's so useful, for example--but we always consider the tradeoffs and see if there's a way to avoid loading part of the stdlib.

In this specific case, I understand this was needed for 1.8 in order to make Mutex available. In rspec-mocks, we had a similar situation, and here's how we dealt with it:

https://github.com/rspec/rspec-mocks/blob/v3.2.0/lib/rspec/mocks/space.rb#L134-L146

This comment has been minimized.

@JoshCheek

JoshCheek Feb 7, 2015

Contributor

Yeah, I like that. Someone somewhere will have a Queue class will have half its methods overridden by the one in thread, and then their program will segfault, just b/c we require thread here >.< I'll do something similar to this. Means 1.8 won't be threadsafe, but 1.8's been end of lifed, so shouldn't be any new development on it, anyway.

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

Means 1.8 won't be threadsafe, but 1.8's been end of lifed, so shouldn't be any new development on it, anyway.

Well, if you use the stdlib Mutex if it's available, then wouldn't 1.8 still be threadsafe? After all, if the thread stdlib hasn't been loaded than the user can't be using multiple threads, right?

@mutex = Mutex.new
end

def for(key, &initializer)

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

Capturing a block and using block.call is much slower than just using yield. See our benchmark:

https://github.com/rspec/rspec-core/blob/v3.2.0/benchmarks/capture_block_vs_yield.rb

Can you change to yield?

def for(key, &initializer)
return @memoized[key][0] if @memoized.key? key

value = initializer.call

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

It looks to me like initializer could get called multiple times. IMO this has the potential to cause buggy behavior:

  • If multiple threads get here, it's non-deterministic which value will actually be memoized and returned by the let-defined method -- it's basically the value for whichever thread gets into the synchronize block first, which we have no control over.
  • Besides computing a value or creating an object, the let definition may cause some side effect, and having it run multiple times could be very problematic. Consider, for example, a typical rails spec that creates an AR record using let and then has later test logic that assumes that record is the only one in the DB table. If initializer.call is executed twice by two different threads, the DB table could have 2 records instead of 1 and the user would get intermittent, non-deterministic test failures.

This comment has been minimized.

@JoshCheek

JoshCheek Feb 7, 2015

Contributor

Without mixing the two, this should be correct behaviour: It is true that a nondeterministic value can be memoized, but not true that a nondeterministic value can be returned. Ie for two threads, the first one to get to the synchronize block wins, so second thread will discard its calculated value and all threads see the same result.
Within the same thread the last one to the memoized block wins so that you can stack calls to let.

We have to invoke the block outside the mutex because if the block calls super and hits another let block, then it will end up back in this same method, basically this:

m = Mutex.new
m.synchronize { m.synchronize { } }

Though as I think through it now, I can see two issues:

  1. If you mix these two (call super() to another let block, with multiple threads), it could do the wrong thing, seeing the memoized value of the supercall from the other thread. Ideally you could lock let such that only one thread could calculate it, but you run into the issue described above. There's Mutex#owned? which might be useful, but it says "This API is experimental, and subject to change" :/
  2. If the calls to let have side effects, you can incur those side effects multiple times, even though both threads will see the same value come back from the let block. Again, the ability to lock to a thread seems like it would fix this.

I have a sense that we can make it work in all these strange cases, and also be simpler. Gonna do right-brain things for a bit and see if it manifests.

memoized[0]
end
end
end

This comment has been minimized.

@myronmarston

myronmarston Feb 7, 2015

Member

On the surface, it looks like this implementation uses two different mechanisms to deal with thread safety issues:

  • Mutex synchronization.
  • Thread.current_object_id

Is there a way we can simplify this to one mechanism? FWIW, I played around with this locally and tried this:

def for(key)
  return @memoized[key] if @memoized.key? key

  @mutex.synchronize do
    return @memoized[key] if @memoized.key? key
    @memoized[key] ||= yield
  end
end

...which I expected to work, but it didn't, and I realized why: when the logic in one let references another let, it hits deadlock. So I realize it may not be simple to get down to just one mechanism :(.

One solution to consider is using ThreadSafe::Cache#fetch_or_store, which would "just work", I think, and require no extra synchronization. That has some other issues, though: we've historically not depended on external gems (with one exception: rspec-expectations uses diff-lcs for diffing) and I'm not sure I want to start doing that. For example, adding another dependency could prevent users from being able to upgrade RSpec if their bundle already depends on an older version of the external gem than what our version constraint says we support.

More thought is needed, I think...

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 7, 2015

One other random thought I just had...if the overhead of the threadsafety is high enough that we want to avoid it when the user isn't using threads, I think we could probably put in a conditional like this:

if Thread.list.one?
  do_it_the_non_threadsafe_way
else
  do_it_the_threadsafe_way
end

That assumes the overhead of Thread.list.one? is less than the overhead of the threadsafety synchronization (which is probably a safe assumption, but we'd want to benchmark Thread.list.one? to see how expensive it is, of course). I believe it would be safe to use the non-threadsafe implementation in that case since the currently executing thread is the only one so while the user might spawn a thread later in the spec, that can't happen until the value is memoized, and once its been memoized, the threaded race conditions are no longer a concern.

myronmarston added a commit that referenced this pull request Feb 7, 2015

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 7, 2015

OK, @JoshCheek, I updated the benchmark to not use the rest of rspec-core. The difference is astounding. It's in the updated-benchmarks-for-pr-1858 branch -- see d371e84. In my benchmarks, threadsafe let is 3x slower in the best case and 85,000 times slower in the worst case. That latter number is so outrageous that it makes me doubt my benchmark but I've reviewed my code multiple times to see if I messed something up and as far as I can tell, I haven't (I even added some verification code to make sure the memoization was working properly in both cases...).

I think what this means is that your benchmark was entirely dominated by the overhead of the rest of RSpec (defining example groups and examples, and running them), whereas mine avoids such overhead entirely, using let directly in custom host classes. If you think about it, the non-threadsafe implementation of let in the "call it 10x" case is basically just a hash lookup which we all know is highly optimized in Ruby so maybe it can be that much faster.

Want to take a look?

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 7, 2015

OK, I found why my benchmark was showing an 85K x difference. Bug on my part. Fixed up in 11ee951. Now it's a 3-4x difference across the board.

Still pretty significant IMO.

@JonRowe

This comment has been minimized.

Copy link
Member

JonRowe commented Feb 8, 2015

Yeah at 3-4x performance difference this would have to be opt in.

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Feb 8, 2015

Not my lib, but I wouldn't personally have an issue with 3 to 4 times slower, simply because we're still talking about thousands to tens of thousands of these per second. I can't think of any lib which would be noticeably impacted. RSpec core, for example, ag '\blet\b' | wc -l finds 310 calls, and ag '\b(it|example|specify).*?[\'"%]\b' | wc -l finds 2210 examples. It wouldn't be noticed even if the test time weren't dominated by IO.

The part that's hard, to me, is making it correct in all cases. It feels like the kind of thing where if you commit a little, you commit a lot. I think you have to lock out other threads to actually be correct with the other constraints, which would require some threadbased abstractions or a dependency (of these two, I typically opt for the former). The Memoized abstraction would be cleaner and simpler once that's there, but it still feels like a lot of overhead that could creep out to other areas, will need to be maintained, and could be difficult to keep correct (esp since JRuby and Rbx are both allowed to fail on CI, and they're the ones that will really stress its correctness).

So, I'm leaning towards not merging, unless others think it won't be as costly as it seems to me.

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 8, 2015

Not my lib, but I wouldn't personally have an issue with 3 to 4 times slower, simply because we're still talking about thousands to tens of thousands of these per second. I can't think of any lib which would be noticeably impacted. RSpec core, for example, ag '\blet\b' | wc -l finds 310 calls, and ag '\b(it|example|specify).*?[\'"%]\b' | wc -l finds 2210 examples. It wouldn't be noticed even if the test time weren't dominated by IO.

Agreed that it's unlikely for users to notice that perf difference, but lots of little choices to allow worse perf add up over time to RSpec being slower. Making something 3-4x slower that's only ever been reported or requested by one user (and is already explicitly documented as being unsupported) is a poor tradeoff, IMO.

That said, the bigger issue here in my mind is the fact that this feels like only a partial threadsafety fix. As we've discussed the fact that the let block can still be evaluated multiple times is significant. I wouldn't want to merge something and announce that let is now threadsafe as long as that is in place.

(esp since JRuby and Rbx are both allowed to fail on CI, and they're the ones that will really stress its correctness)

It's only jruby-head that's allowed to fail on CI, which by definition is an unreleased version of JRuby. We don't allow failure of JRuby in 1.8, 1.9 or 2.0 mode.

RBX is a different case. We've put a ton of effort (much more than the interpreter-specific effort put into MRI or JRuby) into getting green builds but have been foiled by the fact that RBX contains long-standing bugs that cause it to intermittently fail to compile some of our files (see rubinius/rubinius#2934). As soon as RBX becomes as easy to support as JRuby or MRI we'll be happy to remove it from allowed failures.

I think you have to lock out other threads to actually be correct with the other constraints, which would require some threadbased abstractions or a dependency (of these two, I typically opt for the former).

As I mentioned before, I think that ThreadSafe::Cache#fetch_or_store is the right abstraction for this...after all, an atomic fetch_or_store is the exact semantic we are looking for. I suspect that it would be faster than any home-grown solution (as it has some extensions for specific interpreters that are optimized). The extra dependency would be nice to avoid, but if we make this opt-in, it wouldn't have to be a hard dependency; we'd just have to provide a nice "Please install the thread_safe gem or make it available in order to enable thread-safe let" message if the require fails.

@JonRowe

This comment has been minimized.

Copy link
Member

JonRowe commented Feb 8, 2015

(esp since JRuby and Rbx are both allowed to fail on CI, and they're the ones that will really stress its correctness)

Only Rbx is allowed to fail in its entirety, only JRuby head is allowed to fail (like Ruby head in fact).

I can't think of any lib which would be noticeably impacted.

It's not just libraries/gems of course, but a large Rails project would be impacted to and those test suites can be slow enough already!

RSpec core, for example, ag '\blet\b' | wc -l finds 310 calls, and ag '\b(it|example|specify).*?['"%]\b' | wc -l finds 2210 examples. It wouldn't be noticed even if the test time weren't dominated by IO.

Remember that multiple let's can be used multiple times per example... this sort of thing can add up quickly and unless we made it opt in would be imposed on all test suites, not just on ones that would benefit.

So, I'm leaning towards not merging, unless others think it won't be as costly as it seems to me.

There's benefits to being thread safe, and it's something we'd like to eventually see just not at a performance cost to non threaded users at the moment, we're okay with opt in behaviour (we could even consider automatically opting in JRuby / RBX users at a later date).

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 8, 2015

(woops, submitted before my comment was done...)

If we go the route of using thread_safe and making it opt-in, we can consider graduating it to a hard dependency (and making let always threadsafe) in RSpec 4 if it seems like the community is using it a lot and wants that to always be threadsafe.

Anyhow, I also just realized that I think we can simplify the threadsafety logic and fix the "can execute the block multiples times" bug, without getting deadlocks, if we use a separate mutex per let def. Is that worth playing around with?

Also, it appears that while Mutex is not re-entrant, Monitor is -- so I think you could use a monitor instead:

http://japgolly.blogspot.com/2012/04/ruby-mutex-reentrancy.html

(It's slower though, so the perf concern would likely remain).

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Feb 9, 2015

That said, the bigger issue here in my mind is the fact that this feels like only a partial threadsafety fix. As we've discussed the fact that the let block can still be evaluated multiple times is significant. I wouldn't want to merge something and announce that let is now threadsafe as long as that is in place.

Wasn't intended as a criticism, just an observation about the issue of feedback in truly concurrent environments. My gems don't compile on them either, despite a more than satisfactory amount of effort (though, I'll keep coming back to it). Unfortunate that it has to be all or nothing, otherwise we could say "this one can fail, but that one should be solid" and restrict the uncertainty to a subset of the suite.

It's not just libraries/gems of course, but a large Rails project would be impacted to and those test suites can be slow enough already!

I have difficulty seeing this as realistic. It would need to have tens of thousands of tests, a formidable task. and the overhead we're discussing would be a fraction of a second. Purely based on the size of such a suite, it could take between 10 seconds and several minutes, just to load the code. My initial benchmark, took several seconds just to load the one spec, on my shiny laptop with Ruby 2.2, but an app that old and large would probably be on an older slower Ruby. And, given that it's a Rails app that's doing something relevant enough to have this many tests, the cost of loading bundler, the app's code, and especially its the gems, would eclipse it into negligibility.

A single ActiveRecord::Base#save would affect the load time more than every let and implicit subject becoming ten times slower for every invocation across the entire test suite. And on an app as large as this would need to be, there will be an excess of examples where a dev checked a validation by attempting to save it. Or tested that their member banner didn't reappear by traversing signup with Capybara, kicking off emails and saving ten models to the database before rendering three n+1 queries to finally render the banner in question.

I guess this feels to me like Warren Buffet agonizing over, and researching whether to buy gum from the local 7/11 or the grocery store just up the block.

If we go the route of using thread_safe and making it opt-in, we can consider graduating it to a hard dependency (and making let always threadsafe) in RSpec 4 if it seems like the community is using it a lot and wants that to always be threadsafe.

The load time of this lib could be relevant. Which is much more relevant to me, as an avid user of RSpec, than the cost of let ie it is not unlikely that the addition of one dependency increases total time more than every let block in the suite slowing down by a factor of 10. I personally work in Rails with an impressive infrequently, so my interests are more representative of a lib dev than a Rails dev.

if we use a separate mutex per let def. Is that worth playing around with?

Won't hurt to try it.

Also, it appears that while Mutex is not re-entrant, Monitor is -- so I think you could use a monitor instead:

http://japgolly.blogspot.com/2012/04/ruby-mutex-reentrancy.html

(It's slower though, so the perf concern would likely remain).

That's amazing! I had no idea that was even a thing. I'll try and get to it this week, but my classes are starting up again tomorrow, and my exploration/development tends to lull when students are in session.

@JonRowe

This comment has been minimized.

Copy link
Member

JonRowe commented Feb 10, 2015

and the overhead we're discussing would be a fraction of a second.

Look after the pennies and the pounds take care of themselves, if everyone watched out for those "fraction of a second" increases then our test suites would be much faster then they are today.

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Feb 20, 2015

Update: Still working on this, just been a slow (turns out threads are hard :P) and working a lot lately.

Also, monitor uses thread (here), but a quick scan doesn't make it clear why. Anyway, other than the conditional waiting, it seems to just add reentrance to mutexes. So might make sense to copy the pieces that are relevant to a reentrant #synchronize then can get the behaviour without the dependency.

With Mutex, on 1.8.7, it's defined inside of thread.rb, so you can't access it without loading that file. But again, a quick scan of the code looks like it's a relatively small class, built on top of Thread.critical. So, could possibly take the relevant pieces of these two classes and mash them together for this use case specifically.

Unfortunately, I'm not super confident I can install 1.8.7. and this stuff changed a lot since then. ruby-install says it's unsupported, and I assume that if ruby-install won't do it, there's a reason, which makes building it myself sound nontrivial.

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 20, 2015

Thanks for the update. Don't worry about 1.8.7 for now. Once we have a solution we're happy with for other rubies we can consider what needs to change (if anything) for 1.8.7 but it's just as likely to be wasted effort before we've agreed on the general solution.

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Feb 23, 2015

I have it implemented, only depending on Mutex at the moment, should I push to my threadsafe-let-block to update this PR, or open a new one to the updated-benchmarks-for-pr-1858 branch?


Also, it seems thread is loaded by default.

$ chruby-exec 1.9.3 -- ruby -e 'puts $LOADED_FEATURES' | grep thread
# /Users/josh/.rubies/ruby-1.9.3-p545/lib/ruby/1.9.1/thread.rb

$ chruby-exec 2.0.0 -- ruby -e 'puts $LOADED_FEATURES' | grep thread
# /Users/josh/.rubies/ruby-2.0.0-p451/lib/ruby/2.0.0/thread.rb

$ chruby-exec 2.1.2 -- ruby -e 'puts $LOADED_FEATURES' | grep thread
# thread.rb
# /Users/josh/.rubies/ruby-2.1.2/lib/ruby/2.1.0/x86_64-darwin13.0/thread.bundle

$ chruby-exec 2.2.0 -- ruby -e 'puts $LOADED_FEATURES' | grep thread
# thread.rb
# /Users/josh/.rubies/ruby-2.2.0/lib/ruby/2.2.0/x86_64-darwin13/thread.bundle

$ chruby-exec rbx -- ruby -e 'puts $LOADED_FEATURES' | grep thread
# /Users/josh/.rubies/rbx-2.5.0/lib/rubinius/2.5.0/gems/gems/rubysl-thread-2.0.2/lib/rubysl/thread/version.rb
# /Users/josh/.rubies/rbx-2.5.0/lib/rubinius/2.5.0/gems/gems/rubysl-thread-2.0.2/lib/rubysl/thread/thread.rb
# /Users/josh/.rubies/rbx-2.5.0/lib/rubinius/2.5.0/gems/gems/rubysl-thread-2.0.2/lib/rubysl/thread.rb
# /Users/josh/.rubies/rbx-2.5.0/lib/rubinius/2.5.0/gems/gems/rubysl-thread-2.0.2/lib/thread.rb

$ chruby-exec jruby -- ruby -e 'puts $LOADED_FEATURES' | grep thread
# thread.rb
# thread.jar

It appears monitor is, too.

$ chruby-exec 1.9.3 -- ruby -e 'puts $LOADED_FEATURES' | grep monitor

Just to verify it wasn't something wonky in my env, I tried it on a Heroku app, and they were available there, too. I haven't tried digging into core yet to see why.

example_group = RSpec.describe(&describe_body)
ran_successfully = example_group.run RaiseOnFailuresReporter
expect(ran_successfully).to eq true
end

This comment has been minimized.

@myronmarston

myronmarston Feb 26, 2015

Member

This is quite nice and useful...I may want to use this elsewhere :).

This comment has been minimized.

@JoshCheek

JoshCheek Feb 27, 2015

Contributor

Actually, is that RSpec.describe(&describe_body) right? Looking at it now, that probably adds it to the main test suite, as well. I'll look into it, there's probably an obvious way to get this functionality without adding it to the main suite (probably something like ExampleGroup.new).

This comment has been minimized.

@myronmarston

myronmarston Feb 27, 2015

Member

Actually, is that RSpec.describe(&describe_body) right?

Yep. We use this pattern all over the place in rspec-core's specs. It doesn't add it to the main test suite do to our sandboxing:

https://github.com/rspec/rspec-core/blob/95609709de8c5df85867feda0b9573b5e53a14bd/lib/rspec/core/sandbox.rb
https://github.com/rspec/rspec-core/blob/95609709de8c5df85867feda0b9573b5e53a14bd/spec/support/sandboxing.rb

That ensures that every example runs with a fresh copy of RSpec.world such that RSpec.describe registers the example group on the temporary, one-off world.

expect(loaded_filenames).to_not include 'monitor.rb'
expect(loaded_filenames).to_not include 'thread.rb'
expect(loaded_filenames).to_not include 'thread.bundle'
end

This comment has been minimized.

@myronmarston

myronmarston Feb 26, 2015

Member

FWIW, we now have library-wide checks for minimizing the parts of stdlib we load (it was added in the last few weeks). It's defined in rspec-support:

https://github.com/rspec/rspec-support/blob/154c598971338108457329c44d276cd1a64deded/lib/rspec/support/spec/library_wide_checks.rb#L92-L107

That example group gets included here, where we also define the whitelist of allowed stdlibs for rspec-core:

allowed_loaded_features = [
/optparse\.rb/, # Used by OptionParser.
/rbconfig\.rb/, # loaded by rspec-support for OS detection.
/shellwords\.rb/, # used by ConfigurationOptions and RakeTask.
/stringio/, # Used by BaseFormatter.
%r{/fake_libs/}, # ignore these, obviously
]
# JRuby appears to not respect `--disable=gem` so rubygems also gets loaded.
allowed_loaded_features << /rubygems/ if RSpec::Support::Ruby.jruby?
it_behaves_like 'library wide checks', 'rspec-core',
:preamble_for_lib => [
# rspec-core loads a number of external libraries. We don't want them loaded
# as part of loading all of rspec-core for these specs, for a few reasons:
#
# * Some external libraries issue warnings, which we can't do anything about.
# Since we are trying to prevent _any_ warnings from loading RSpec, it's
# easiest to avoid loading those libraries entirely.
# * Some external libraries load many stdlibs. Here we allow a known set of
# directly loaded stdlibs, and we're not directly concerned with transitive
# dependencies.
# * We're really only concerned with these issues w.r.t. rspec-mocks and
# rspec-expectations from within their spec suites. Here we care only about
# rspec-core, so avoiding loading them helps keep the spec suites independent.
# * These are some of the slowest specs we have, and cutting out the loading
# of external libraries cuts down on how long these specs take.
#
# To facilitate the avoidance of loading certain libraries, we have a bunch
# of files in `support/fake_libs` that substitute for the real things when
# we put that directory on the load path. Here's the list:
#
# * coderay -- loaded by the HTML formatter if availble for syntax highlighting.
# * drb -- loaded when `--drb` is used. Loads other stdlibs (socket, thread, fcntl).
# * erb -- loaded by `ConfigurationOptions` so `.rspec` can use ERB. Loads other stdlibs (strscan, cgi/util).
# * flexmock -- loaded by our Flexmock mocking adapter.
# * json -- loaded by the JSON formatter, loads other stdlibs (ostruct, enc/utf_16le.bundle, etc).
# * minitest -- loaded by our Minitest assertions adapter.
# * mocha -- loaded by our Mocha mocking adapter.
# * rake -- loaded by our Rake task. Loads other stdlibs (fileutils, ostruct, thread, monitor, etc).
# * rr -- loaded by our RR mocking adapter.
# * rspec-mocks -- loaded by our RSpec mocking adapter.
# * rspec-expectations -- loaded by the generated `spec_helper` (defined in project_init).
# * test-unit -- loaded by our T::U assertions adapter.
#
"$LOAD_PATH.unshift '#{fake_libs}'",
# Many files assume this has already been loaded and will have errors if it has not.
'require "rspec/core"',
# Prevent rspec/autorun from trying to run RSpec.
'RSpec::Core::Runner.disable_autorun!'
], :skip_spec_files => %r{/fake_libs/}, :allowed_loaded_feature_regexps => allowed_loaded_features do
if RUBY_VERSION == '1.8.7'
before(:example, :description => /(issues no warnings when the spec files are loaded|stdlibs)/) do
pending "Not working on #{RUBY_DESCRIPTION}"
end
elsif RUBY_VERSION == '2.0.0' && RSpec::Support::Ruby.jruby?
before(:example) do
skip "Not reliably working on #{RUBY_DESCRIPTION}"
end
end
end

So this spec isn't needed. I'm curious about why thread/monitor are getting loaded...I'll take a look.

On a side note, your 3 expectations can be collapsed into one:

expect(loaded_filenames).to_not include('monitor.rb', /thread/)

This comment has been minimized.

@myronmarston

myronmarston Feb 26, 2015

Member

So I looked into what's loading monitor and thread...

  • Rubygems loads monitor. Using --disable=gem bypasses that (which, BTW, our "minimizes stdlibs" spec does to prevent rubygems pollution).
  • Rake loads thread and monitor. rspec-core doesn't generally load rake -- you have to specifically require rspec/core/rake_task -- but in the spec suite it's loaded because it tests the rake task stuff.
  • The DRB support likewise loads DRB (but again, it's not loaded if you're not using the drb option) and DRB loads thread.

This comment has been minimized.

@JoshCheek

JoshCheek Feb 27, 2015

Contributor

Yeah, I saw the rake one, commented on it in the "Segregating env from stdlib" section here. I'm mostly uncomfortable with it loading because it reduces my confidence that this code is actually independent. I can try to either move these tests, or those tests out of the main process (think I saw the suite does this in a few places already). Of those two options, I'd prefer to move the rake and drb tests out, because then it means that we can guarantee all the specs in the main process run without those stdlib additions (ie segregate things that dirty the environment rather than things we want to make sure run in a clean environment). This seems worthwhile, I assume it's the motivation behind the "library wide checks". Though, I'm confused about how those pass (haven't looked into them yet) given that monitor and thread do get required.

This comment has been minimized.

@myronmarston

myronmarston Feb 27, 2015

Member

I think the piece you're missing is our use of "fake libs" for those specs:

# rspec-core loads a number of external libraries. We don't want them loaded
# as part of loading all of rspec-core for these specs, for a few reasons:
#
# * Some external libraries issue warnings, which we can't do anything about.
# Since we are trying to prevent _any_ warnings from loading RSpec, it's
# easiest to avoid loading those libraries entirely.
# * Some external libraries load many stdlibs. Here we allow a known set of
# directly loaded stdlibs, and we're not directly concerned with transitive
# dependencies.
# * We're really only concerned with these issues w.r.t. rspec-mocks and
# rspec-expectations from within their spec suites. Here we care only about
# rspec-core, so avoiding loading them helps keep the spec suites independent.
# * These are some of the slowest specs we have, and cutting out the loading
# of external libraries cuts down on how long these specs take.
#
# To facilitate the avoidance of loading certain libraries, we have a bunch
# of files in `support/fake_libs` that substitute for the real things when
# we put that directory on the load path. Here's the list:
#
# * coderay -- loaded by the HTML formatter if availble for syntax highlighting.
# * drb -- loaded when `--drb` is used. Loads other stdlibs (socket, thread, fcntl).
# * erb -- loaded by `ConfigurationOptions` so `.rspec` can use ERB. Loads other stdlibs (strscan, cgi/util).
# * flexmock -- loaded by our Flexmock mocking adapter.
# * json -- loaded by the JSON formatter, loads other stdlibs (ostruct, enc/utf_16le.bundle, etc).
# * minitest -- loaded by our Minitest assertions adapter.
# * mocha -- loaded by our Mocha mocking adapter.
# * rake -- loaded by our Rake task. Loads other stdlibs (fileutils, ostruct, thread, monitor, etc).
# * rr -- loaded by our RR mocking adapter.
# * rspec-mocks -- loaded by our RSpec mocking adapter.
# * rspec-expectations -- loaded by the generated `spec_helper` (defined in project_init).
# * test-unit -- loaded by our T::U assertions adapter.
#
"$LOAD_PATH.unshift '#{fake_libs}'",

When that library wide check runs it doesn't actually load drb and rake...it loads https://github.com/rspec/rspec-core/blob/95609709de8c5df85867feda0b9573b5e53a14bd/spec/support/fake_libs/rake.rb and https://github.com/rspec/rspec-core/blob/95609709de8c5df85867feda0b9573b5e53a14bd/spec/support/fake_libs/drb/drb.rb instead. That way, we don't have to consider stdlibs loaded by those libraries.

This comment has been minimized.

@JoshCheek

JoshCheek Feb 27, 2015

Contributor

I see. The downside of this, though, is that RSpec could could use those libs unintentionally and pass b/c they're available in its test env, but not have them available in the user's test env. Unless I'm just still confused about how it works.

This comment has been minimized.

@myronmarston

myronmarston Feb 27, 2015

Member

I see. The downside of this, though, is that RSpec could could use those libs unintentionally and pass b/c they're available in its test env, but not have them available in the user's test env. Unless I'm just still confused about how it works.

I think our cukes guard against this -- each scenario writes out a file, runs it via the rspec command and has expectations on the produced output. When the cuke does that, it's shelling out and running a new ruby process and booting rspec from scratch to run the one file -- so it has no pollution from any of our test environment stuff. We don't exhaustively test every code path via cukes obviously (that would be horrible) but they cover a large percentage of the features, enough that I'm not concerned about this in practice.

def initialize
@bodies = {}
@threads = []
@queue = [] # we may not have thread stdlib required, so may not have Queue class

This comment has been minimized.

@myronmarston

myronmarston Feb 26, 2015

Member

We care about minimizing stdlibs loaded from RSpec itself, but loading stdlibs in specs to help you test things like this is fine -- so I'd recommend just using Queue over [] with a mutex. Stdlibs or other libraries loaded in the specs are isolated from the "minimize loaded stdlibs" library-wide spec I mentioned above because that one shells out and starts a new ruby process, loading all of the lib files of the repo and thus avoiding pollution by spec files loading needed libraries.

This comment has been minimized.

@JoshCheek

JoshCheek Feb 27, 2015

Contributor

I'm going to have to look into this in more detail this weekend, I'm clearly confused about how it works, as I'd assume this would cause library wide checks to fail.

This comment has been minimized.

@myronmarston

myronmarston Feb 27, 2015

Member

The spec that enforces the whitelist of allowed stdlibs loads only the lib files of the project, not the spec files, so libraries loaded in spec files to facilitiate testing are not considered.

enqueue { watch_for_sleep(thread, &cb) }
end
end
end

This comment has been minimized.

@myronmarston

myronmarston Feb 26, 2015

Member

I'm struggling to understand this :(. That's probably partially the fact that it's 12:40 AM here. I suspect it'll be easier to understand once you switch to a proper thread safe Queue.

This comment has been minimized.

@JoshCheek

JoshCheek Feb 27, 2015

Contributor

If you're referring to watch_for_sleep, it check to see if the thread died (false) or errored (nil), which means it won't ever sleep, so in this case it returns, which means it won't continue watching. If the status is "sleep", then it has found what it's watching for, so it invokes the callback and returns, which causes it to stop watching. If the thread is not dead/errored/asleep, then it needs to keep watching, so it enqueues another call to watch_for_sleep, which means that the worker will eventually check again.

If you're referring to the implementation of the class as a whole, then yeah, it's a bit much, I struggled to understand it, too. It took 12 hours to write the thing, because it took that long to get my brain to really consider the various implications of concurrency. My way of thinking about it is that it's probably a shitty implementation of what node does. If there's some good way to delete it, or simplify it, I would love to see that. Right now, I can't really think of any, and I'm only 80-90% confident that it actually works (I test it by running it on rbx 20 times in a row without deadlocks or failures).

This comment has been minimized.

@myronmarston

myronmarston Feb 27, 2015

Member

You know, this might be worthwhile extracting into its own gem (hint, hint) :).

This comment has been minimized.

@JoshCheek

JoshCheek Feb 27, 2015

Contributor

:P

to raise_error(ArgumentError, /bad_key/)
end
end
end

This comment has been minimized.

@myronmarston

myronmarston Feb 26, 2015

Member

This is a fine place for the specs for now. Long term, I suspect this may migrate into rspec-support (particularly if we want to use it from other libraries) but it's fine here for now.

it 'raises ArgumentError when told to resume on an unknown status' do
order.declare(:t) { }
expect { order.pass_to :t, :resume_on => :bad_status }.
to raise_error(ArgumentError, /bad_status/)

This comment has been minimized.

@myronmarston

myronmarston Feb 26, 2015

Member

Stylistically, moving the to matcher bit onto its own line is significant departure from the style we use everywhere else. We tend to use this instead:

expect {
  order.pass_to :t, :resume_on => :bad_status
}.to raise_error(ArgumentError, /bad_status/)

You mind changing to that style for consistency with the rest of the project?

This comment has been minimized.

@JoshCheek

JoshCheek Feb 27, 2015

Contributor

Will do.

JoshCheek added a commit to JoshCheek/thread_order that referenced this pull request Feb 28, 2015

JoshCheek added a commit to JoshCheek/rspec-core that referenced this pull request Mar 15, 2015

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Mar 15, 2015

#1858 (comment)
You know, this might be worthwhile extracting into its own gem (hint, hint) :).

Done, gem is called thread_order. Passes on CI against all the rubies RSpec tests against.

#1858 (comment)

> expect {
>   order.pass_to :t, :resume_on => :bad_status
> }.to raise_error(ArgumentError, /bad_status/)

You mind changing to that style for consistency with the rest of the project?

This was extracted into the thread_order gem.

#1858 (comment)
This is a fine place for the specs for now. Long term, I suspect this may migrate into rspec-support (particularly if we want to use it from other libraries) but it's fine here for now

Also moved into thread_order gem.

#1858 (comment)
Nevermind -- I have no idea what I was thinking but I'm convinced now I was wrong. Sorry for the false alarm, though.

I do have one suggestion for a potential perf improvement...I think fetch_or_store could be:

def fetch_or_store(key)
  @memoized.fetch(key) do
    @mutex.synchronized do
      @memoized.fetch(key) { @memoized[key] = yield }
    end
  end
end

That way the synchronization cost is only paid when the @memoized doesn't yet have the value. On future accesses, it short-circuits and just returns the value it already has w/o synchronizing.

Done.


I'll figure out how to squash it all together once I get the thumps up (it's not a skill I have, b/c it's not a thing I normally do).

One thing we'd discussed earlier that I'm not sure about yet is whether we want to make it optional/configurable. Personally, I don't, since I can't imagine it making a difference for any real-world test suite, but not my call. (If you have a list of intense test suites that use RSpec, I'll run it against them and document the results).

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Mar 15, 2015

Great work, @JoshCheek!

Done, gem is called thread_order. Passes on CI against all the rubies RSpec tests against.

Thanks. I can see myself using your gem for other projects :).

I'll figure out how to squash it all together once I get the thumps up (it's not a skill I have, b/c it's not a thing I normally do).

Given it's a workflow you're not used to, I'm happy to do that just before merge, so don't worry about it unless you specifically want to gain some experiencing using git's interactive rebase features.

One thing we'd discussed earlier that I'm not sure about yet is whether we want to make it optional/configurable. Personally, I don't, since I can't imagine it making a difference for any real-world test suite, but not my call. (If you have a list of intense test suites that use RSpec, I'll run it against them and document the results).

I'm still on the fence about this, so let me explain my thought process here a bit...

  • I was curious if the extra oveheard was primarily the mutex.synchronize call or the added overhead of the new Memoized class -- when using a raw hash you're using a class written in C, and once we start using Memoized there's added overhead of rubyland stuff. I definitely want rspec to support threadsafe let in some form, so if most of the overhead was in introduction of the new Memoized abstraction, and not the use of synchronize, we'd just make it always threadsafe with no config option.
  • OTOH, if most of the overhead was from synchronize, then I think it is worth having a config option (regardless of which way we default it), given how trivial it is to support simply by substituting this implementation of Memoized:
  class NonThreadSafeMemoized
    def initialize
      @memoized = {}
    end

    def fetch_or_store(key)
      @memoized.fetch(key) { @memoized[key] = yield }
    end
  end

We just need to make a config object that picks with memoized implementation is used. Quite trivial.

  • To answer this question, I worked off your benchmark and updated it to compare the two existing implementations against a non-threadsafe-via-alternate-memoized-implementation version. You can see my changes in 9f01d6f. The TL;DR is that synchronize is the majority of the overhead, especially for common "call the let method once" case. Threadsafe is 1.99x slower and non-threadsafe-via-config is 1.03x slower.
  • So...I think it's definitely worth having a config option. Now the question is, do we default threadsafety on or off? I'm coming around to the idea of defaulting to it on since correctness trumps an imperceptible perf diff. I may yet flip flop on that though :).

So I think there are still a couple outstanding TODOs before this is ready to merge:

  • Address the potential race condition from @__memoized ||= Memoized.new since ||= is not threadsafe (prior discussion). Seems like the simplest solution is to initialize @__memoized when the example group is instantiated.
  • Add a config option (maybe called config.threadsafe_let?). I suggest renaming Memoized to ThreadsafeMemoized and creating a new NonThreadsafeMemoized that lacks the mutex and synchronize. The config option would just change what class gets instantiated for @__memoized.
  • Update the docs for let to reflect the changes.
  • Squash it into a smaller number of commits before merge.

You've done a ton of great work here that we definitely want to incorporate in the next release, so if you hit a point where you don't have the time or lack the desire/energy to address these last TODOs let us know and one of us can take it across the finish line. We're quite happy to have you finish it, of course :).

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Mar 15, 2015

Given it's a workflow you're not used to, I'm happy to do that just before merge, so don't worry about it unless you specifically want to gain some experiencing using git's interactive rebase features.

I'll give it a shot, I've found that just sucking it up and tackling things I fear is usually worthwhile. I was afraid of threads for half a decade -- but as of right now, they seem more like a puzzle, and have opened up some interesting domains.

Thanks. I can see myself using your gem for other projects :).

Cool :) I'll probably do a bit more work on it, as well. Its current incarnation is mostly to directly support the use cases I had for writing these tests.

Address the potential race condition from @__memoized ||= Memoized.new since ||= is not threadsafe (prior discussion)

Ahh, yes, I knew I was forgetting some things (I've found it difficult to track inline discussions, hence reverting to block-quoting)

Add a config option (maybe called config.threadsafe_let?). I suggest renaming Memoized to ThreadsafeMemoized and creating a new NonThreadsafeMemoized that lacks the mutex and synchronize. The config option would just change what class gets instantiated for @__memoized.

Will do.

Update the docs for let to reflect the changes.

Can you verify that this would be: RDocs and Relish docs for let, let!, subject, and subject!. The RDocs I expect to be generated from inline comments, probably using the YARD syntax. The Relish app, I believe is generated from cukes. Oh, probably the configuration options, as well.

You've done a ton of great work here that we definitely want to incorporate in the next release, so if you hit a point where you don't have the time or lack the desire/energy to address these last TODOs let us know and one of us can take it across the finish line. We're quite happy to have you finish it, of course :).

Ty. Next week will be tight, big projects and assessments due, so all the students get freaked out and need attention, 12 hr days, probably. But after that, I'll be on break again, and have time to allocate to projects and PRs.

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Mar 15, 2015

Can you verify that this would be: RDocs and Relish docs for let, let!, subject, and subject!. The RDocs I expect to be generated from inline comments, probably using the YARD syntax. The Relish app, I believe is generated from cukes. Oh, probably the configuration options, as well.

Yep.

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Mar 29, 2015

I'll squash all these once I get the final thumbs up.

Things worth looking at before giving me the thumbs up:

  • The values are made threadsafe in an initialize method that is injected into the hierarchy. This seemed like the least invasive way to pull it off (could maybe do a before filter, but ordering could get messed up). I think I'd like to hear someone say "that seems reasonable"

  • For the config option, I went with --threadsafe I didn't want to use a name based on let blocks, because it works for all memoized helpers. Additionally, if there are other areas where RSpec is not threadsafe, then they may wish to move in this direction, and it seemed nice that there would already be an option for them to hook into. But, it might be implying more than it can deliver.

  • See JoshCheek@2daae80, I made a minor change to the implementation of the test (instead of setting RSpec.configuration.threadsafe and calling the parent initialize, which sets @__memoized based on the config option, I just directly set the one I was expecting). This minor change led to an average reduction of 21% in terms of how much slower it was.

    This implies that the tests are so focused that we're measuring things like the cost of a method call. It's really hard for me to convince myself that this cost is greater than the cost to future momentum from having to maintain an additional parse option, config option, and parallel implementations.

    If we had a test that measured the cost of one method call, and another that measured the cost of two, we would see that it was 2x slower, but I'd be hesitant to infer that we should inline every method.

  • Documentation added to

    I omitted let! and subject! because they sate that they are just like their non-bang'd counterparts. I didn't directly place a cuke on the threadsafe option, because it would be the same as the one shown in the let block. I didn't add a cuke for subject because it would also be basically the same as the let block one.

  • I dislike that the name of this variable is RSpec::Core::MUTEX, because when 1.8.7 is dropped, it would be nice to be able to delete all that code, and have anything referencing Mutex naturally switch to the one on Object. With this, any code referencing that variable potentially breaks. IDK if that's a real concern, I named it this way b/c one of the robots was yelling at me.

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Mar 30, 2015

Thanks, @JoshCheek. Your work here is great.

There are a few minor things that I'd like to see change a bit before we release with this, but at this point I think it's going to be most efficient to get this squashed and merged as-is, and then I can quickly do a follow-up PR with the few changes I have in mind (and /cc you on it so you can leave any feedback you want on my changes).

So squash away.

Make memoized helpers threadsafe
See #1858 for discussion.

The unsquashed version can be seen at
https://github.com/JoshCheek/rspec-core/tree/threadsafe-let-block-unsquashed
in case the intermediate state and thoughts have value.

Motivation
----------

When working in a truly threaded environment (e.g. Rbx and JRuby),
you can write a test like the one below, which should pass.
But, it will fail sometimes, because two threads request the uninitialized counter
concurrently. The first one to receive the value will then be
incrementing a counter, that is later overwritten when the second
thread's let block returns.

You can verify this by running the code sample below in Rubinius.
After some number of attempts, the value will be less than 10k.
If you then make the `let` block a `let!` block, it will memoize
before the threads are created, and the values will be correct again.
While this is a reasonable solution, it is incredibly confusing
when it arises (I spent a lot of hours trying to find the bug in my code),
and requires that the user understand it's something they need to
be aware of and guard against.

```ruby
class Counter
  def initialize
    @mutex = Mutex.new
    @count = 0
  end

  attr_reader :count

  def increment
    @mutex.synchronize { @count += 1 }
  end
end

RSpec.describe Counter do
  let(:counter) { Counter.new }

  it 'increments the count in a threadsafe manner' do
    threads = 10.times.map do
      Thread.new { 1000.times { counter.increment } }
    end
    threads.each &:join
    expect(counter.count).to eq 10_000
  end
end
```

Relevant Changes
----------------

* Adds `--[no]-threadsafe` command-line option
* `RSpec::Core::Configuration#threadsafe{?,=}`
  (notation there is from bash expansion)
  setting, defaults to `true`
* Mamoized is a threadsafe object instead of a hash,
  so rename `RSpec::Core::MemoizedHelpers::ContextHookMemoizedHash`
  to        `RSpec::Core::MemoizedHelpers::ContextHookMemoized`

  Create an object because only #fetch was being called on the hash,
  and to make that threadsafe required adding a new method.
  At that point, there is no value in having it subclass Hash,
  having to support an entire API that nothing else uses.
  So just make it an object which wraps a hash and presents a memoized
  getter/setter. Also named the method in such a way as to make it
  very clear that it's not a hash, so a dev won't mistakenly think
  they are working with one when they aren't.
* Adds private class `RSpec::Core::ReentrantMutex`,
  which is basically just `Monitor` from the stdlib.
* `RSpec::Core::ExampleGroup#initialize` now calls super so that
  `RSpec::Core::MemoizedHelpers` can initialize the memoized helper
  when the example is instantiated, rather than when it is accessed,
  as this is not threadsafe.

Context and daydreams
---------------------

* PR can be seen unsquashed at:
  https://github.com/JoshCheek/rspec-core/tree/threadsafe-let-block-unsquashed
  This is my first "real" squash, and it wasn't super smooth. I think I
  got it right, in the end, but if you're looking at it going "wtf?", and
  want some more context into the squash itself, I documented it at:
  https://gist.github.com/JoshCheek/017c399641a44286428b
  Also, while I'm pretty sold against anything that changes history,
  there's probably a way to do this that's better than what I did,
  so if you read that and have insights, shoot em my wya, I'd love to get better!
* This does not add any stedlib dependencies as they affect the test
  environment of all users.
* The threads need to be reentrant, which will allow a let block in a thread to
  access its parent let block. This can be achieved with Monitor from
  the stdlib. However, to avoid the dependency, the code was copied from
  https://github.com/ruby/ruby/blob/eb7ddaa3a47bf48045d26c72eb0f263a53524ebc/lib/monitor.rb#L9
  and pasted/edited into `lib/rspec/core/reentrant_mutex.rb`
  This way the user's test environment is preserved, but we still get
  reentrant mutexes.
* Reentrant mutexes are built on top of normal mutexes. These are in
  core now, but for 1.8.7, were defined in the stdlib's thread.rb
  So, similarly, copy that code into `RSpec::Core::ReentrantMutex::MUTEX`
  (capitalization due to Rubocop rules),
  with a note stating that it should be deleted once 1.8 support is dropped.
  If there is a Mutex available already, though, as on 1.9.x+, it will use that.
* Adds a development dependency on a gem, thread_order, which I extracted out of this work.
  Its purpose was to ease the difficulty and opacity of specifying what should
  happen and when, with regards to getting the threads into situations to illustrate
  expected behaviour. It similarly works on 1.8.7 - 2.2, and on JRuby and Rbx,
  without dependencies on the stdlib.
* Add benchmark for threadsafe let block, here is a summary:

  ```
  MRI 2.2

  1 call to let -- each sets the value
    non-threadsafe (original):   830988.5 i/s
    non-threadsafe (config)  :   665661.9 i/s - 1.25x slower
    threadsafe               :   323574.9 i/s - 2.57x slower
  10 calls to let -- 9 will find memoized value
    non-threadsafe (original):   346302.0 i/s
    non-threadsafe (config)  :   309970.2 i/s - 1.12x slower
    threadsafe               :   208946.3 i/s - 1.66x slower
  1 call to let which invokes super
    non-threadsafe (original):   591906.3 i/s
    non-threadsafe (config)  :   511295.0 i/s - 1.16x slower
    threadsafe               :   246079.6 i/s - 2.41x slower
  10 calls to let which invokes super
    non-threadsafe (original):   297422.6 i/s
    non-threadsafe (config)  :   264045.8 i/s - 1.13x slower
    threadsafe               :   170853.1 i/s - 1.74x slower
  ```

  The threasafe let is 1.5 to 2.5 times slower than the original hash
  implementation. The hash alternative is 1.1 to 1.25 times slower.
  If this matters for you, you can configure which one you want to use.

  Either way, you can call the method defined by a let block hundreds of
  thousands of times a second.

@JoshCheek JoshCheek force-pushed the JoshCheek:threadsafe-let-block branch from 2f50d20 to ffe00a1 Apr 3, 2015

@JoshCheek

This comment has been minimized.

Copy link
Contributor

JoshCheek commented Apr 3, 2015

Whew, think I finally got it 😌💦 Also, nice to see the comments on the unsquashed commits weren't lost. Went back and double checked against the repo that had initially hit the issue, and it's passing just fine with the new code (hangs with the old code), so that's encouraging, as well ^_^

I'm opposed to code ownership as it seems to only hinder worthwhile changes, so don't hesitate to change / remove / add more to anything I've done ^_^

myronmarston added a commit that referenced this pull request Apr 3, 2015

Merge pull request #1858 from JoshCheek/threadsafe-let-block
Make memoized helpers threadsafe

@myronmarston myronmarston merged commit 76c0259 into rspec:master Apr 3, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Apr 3, 2015

Thanks!

myronmarston added a commit that referenced this pull request Apr 3, 2015

Tweak a few things about the threadsafe let solution from #1858.
1). Remove `--threadsafe` CLI option.

In general, we don't expose every config option via the CLI.
We want to make it easy for users to run `rspec --help` and
find the options that they are commonly going to want to customize
for a particular CLI run. I don't think `--threadsafe` is one of
those -- it's more something that'll be turned off globally for the
project or not touched at all, and as such, it adds noise to
the `--help` output to include it.

2). Extract Mutex class into its own file.

- Remove need for use of `MUTEX` over `Mutex`.
- No reason to force Ruby to parse the code
  for 1.8.7 Mutex implementation on other Rubies.

3). Remove threadsafe scenario.

A note about threadsafey is sufficient for the docs. Having
a full threadsafety example spec in the scenario is more
detail than users are likely to want to read.
@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Apr 3, 2015

BTW, I love your epic commit message :).

myronmarston added a commit that referenced this pull request Apr 3, 2015

Tweak a few things about the threadsafe let solution from #1858.
1). Remove `--threadsafe` CLI option.

In general, we don't expose every config option via the CLI.
We want to make it easy for users to run `rspec --help` and
find the options that they are commonly going to want to customize
for a particular CLI run. I don't think `--threadsafe` is one of
those -- it's more something that'll be turned off globally for the
project or not touched at all, and as such, it adds noise to
the `--help` output to include it.

2). Extract Mutex class into its own file.

- Remove need for use of `MUTEX` over `Mutex`.
- No reason to force Ruby to parse the code
  for 1.8.7 Mutex implementation on other Rubies.

3). Remove threadsafe scenario.

A note about threadsafey is sufficient for the docs. Having
a full threadsafety example spec in the scenario is more
detail than users are likely to want to read.

myronmarston added a commit that referenced this pull request Apr 3, 2015

Merge pull request #1919 from rspec/threadsafe-let-followups
Tweak a few things about the threadsafe let solution from #1858.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment