Skip to content
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

Thread safety improvements #8510

Merged
merged 1 commit into from Dec 14, 2012

Conversation

thedarkone
Copy link
Contributor

I don't expect this to be insta-merged - discussion, code review and suggestions are welcome.

The problem is that there are heaps of non thread safe usage of Hashes in Rails and the surrounding gems. This needs to be fixed. Instead of littering the whole code base with the synchronize {} blocks, it is better done by creating a fully thread-safe hash-like data structure.

This is where ThreadSafe::Cache comes in. It is:

  • a thread-safe hash-like data structure
  • reads are lock-free
  • writes might be using locks, but this is fixable in the future
  • storing an item in Cache constitutes safe publication
  • reads have volatile semantics (allows for properly done double-checked locking idiom)
  • writes have volatile semantics

Implementation details are as follows:

Why is it better than explicit synchronisation?

A thread-safe hash is a very useful abstraction, it simplifies the code and allows to avoid worrying about thread-safety most of the time. Explicit locking is not so easily done and is prone to missing some code paths. As the amount of explicit locks increases, so does the probability of deadlocks. Locks are slightly slower than lock-free volatile reads and scale horribly as the concurrency level increases.

Previous pull request: #6917.

@steveklabnik
Copy link
Member

The problem is that there are heaps of non thread safe usage of Hashes in Rails and the surrounding gems. This needs to be fixed. Instead of littering the whole code base with the synchronize {} blocks, it is better done by creating a fully thread-safe hash-like data structure.

👍 👍 👍

/cc @tenderlove

@alup
Copy link
Contributor

alup commented Dec 13, 2012

👍

@glebm
Copy link
Contributor

glebm commented Dec 13, 2012

Wow this is awesome, and anyone who has jruby doing some serious concurrency knows why!
🍻 🎱 @thedarkone

@PragTob
Copy link
Contributor

PragTob commented Dec 13, 2012

👍

@mperham
Copy link
Contributor

mperham commented Dec 13, 2012

Oh my.

@tenderlove
Copy link
Member

I'm happy with this. I think we can rm the parameters filter cache, but I can take care of that. @thedarkone thank you for the work! Can you squash these commits and I'll merge this in?

@samgranieri
Copy link
Contributor

@tenderlove so we should see this in rails 4?
I'm moving off resque to sidekiq and I love getting more bang for the buck in ruby.

Summary of the changes:
 * Add thread_safe gem.
 * Use thread safe cache for digestor caching.
 * Replace manual synchronization with ThreadSafe::Cache in Relation::Delegation.
 * Replace @attribute_method_matchers_cache Hash with ThreadSafe::Cache.
 * Use TS::Cache to avoid the synchronisation overhead on listener retrieval.
 * Replace synchronisation with TS::Cache usage.
 * Use a preallocated array for performance/memory reasons.
 * Update the controllers cache to the new AS::Dependencies::ClassCache API.
   The original @controllers cache no longer makes much sense after @tenderlove's
   changes in 7b6bfe8 and f345e23.
 * Use TS::Cache in the connection pool to avoid locking overhead.
 * Use TS::Cache in ConnectionHandler.
@thedarkone
Copy link
Contributor Author

@tenderlove squashed.

@tenderlove
Copy link
Member

Thanks!

tenderlove added a commit that referenced this pull request Dec 14, 2012
@tenderlove tenderlove merged commit 4921929 into rails:master Dec 14, 2012
@steveklabnik
Copy link
Member

🤘

@gaurish
Copy link
Contributor

gaurish commented Dec 17, 2012

There are issues with thread_safe when trying to create new rails app based on master(commit d8607c1), ruby can't find thread_safe & LoadError occurs

full stack trace below.

 $ ruby ~/code/repo/rails/railties/bin/rails new routers --dev
/home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require': cannot load such file -- thread_safe (LoadError)
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/code/repo/rails/activesupport/lib/active_support/inflector/inflections.rb:1:in `<top (required)>'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/code/repo/rails/activesupport/lib/active_support/inflector/methods.rb:3:in `<top (required)>'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/code/repo/rails/activesupport/lib/active_support/dependencies/autoload.rb:1:in `<top (required)>'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/code/repo/rails/activesupport/lib/active_support.rb:25:in `<top (required)>'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/code/repo/rails/railties/lib/rails/generators.rb:4:in `<top (required)>'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/code/repo/rails/railties/lib/rails/commands/application.rb:22:in `<top (required)>'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/code/repo/rails/railties/lib/rails/cli.rb:15:in `<top (required)>'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/.rvm/rubies/ruby-1.9.3-p286-perf/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
        from /home/gaurish/code/repo/rails/railties/bin/rails:7:in `<main>'

@thedarkone
Copy link
Contributor Author

@gaurish I think you need to run bundle when using bin/rails commands directly from a rails repo, since because you haven't installed the rails gem its dependencies (thread_safe) haven't been installed either. In fact I think you should be using bundle exec bin/rails in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants