Replace global Hash usage with TS::Cache #209

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@thedarkone

This adds thread_safe gem dependency, do you want me to regenerate Gemfile and arel.gemspec?

@ernie

This comment has been minimized.

Show comment
Hide comment
@ernie

ernie Sep 30, 2013

Collaborator

@thedarkone the dispatch cache is global, but also deterministic. Is there some real problem you can demonstrate this solving? It adds a lot of overhead, otherwise.

Collaborator

ernie commented Sep 30, 2013

@thedarkone the dispatch cache is global, but also deterministic. Is there some real problem you can demonstrate this solving? It adds a lot of overhead, otherwise.

@thedarkone

This comment has been minimized.

Show comment
Hide comment
@thedarkone

thedarkone Sep 30, 2013

@ernie Hash is not thread safe, that's about it. What this means is that it cannot be modified concurrently.

Try running (a couple of times) this gist on Rubinius for example: https://gist.github.com/thedarkone/559673.

@ernie Hash is not thread safe, that's about it. What this means is that it cannot be modified concurrently.

Try running (a couple of times) this gist on Rubinius for example: https://gist.github.com/thedarkone/559673.

@ernie

This comment has been minimized.

Show comment
Hide comment
@ernie

ernie Sep 30, 2013

Collaborator

Right, but what I mean is that since the "downside" of this is that we potentially do a few more method lookups than necessary, but no real bugs occur, I'm not sure we need this patch, which adds a dependency and additional overhead.

Unless there is a reproducible bug, I think I'm -1 on this one.

Collaborator

ernie commented Sep 30, 2013

Right, but what I mean is that since the "downside" of this is that we potentially do a few more method lookups than necessary, but no real bugs occur, I'm not sure we need this patch, which adds a dependency and additional overhead.

Unless there is a reproducible bug, I think I'm -1 on this one.

@thedarkone

This comment has been minimized.

Show comment
Hide comment
@thedarkone

thedarkone Sep 30, 2013

Right, but what I mean is that since the "downside" of this is that we potentially do a few more method lookups than necessary, but no real bugs occur, I'm not sure we need this patch, which adds a dependency and additional overhead.

The "downside" you are describing is what is going to happen after my patch, since ThreadSafe::Cache is thread safe.

Before the patch you're concurrently modifying a Hash instance, which is not thread safe. You might get random errors or all CPUs stuck in a infinite loop. Did you run my gist?

Right, but what I mean is that since the "downside" of this is that we potentially do a few more method lookups than necessary, but no real bugs occur, I'm not sure we need this patch, which adds a dependency and additional overhead.

The "downside" you are describing is what is going to happen after my patch, since ThreadSafe::Cache is thread safe.

Before the patch you're concurrently modifying a Hash instance, which is not thread safe. You might get random errors or all CPUs stuck in a infinite loop. Did you run my gist?

@ernie

This comment has been minimized.

Show comment
Hide comment
@ernie

ernie Sep 30, 2013

Collaborator

I did not. I am on ios at the moment and unable to do so. If that is the case, then it would be a problem. I expected you were referring to a concern about potentially "losing" the capture of a cached value.

Collaborator

ernie commented Sep 30, 2013

I did not. I am on ios at the moment and unable to do so. If that is the case, then it would be a problem. I expected you were referring to a concern about potentially "losing" the capture of a cached value.

@thedarkone

This comment has been minimized.

Show comment
Hide comment
@thedarkone

thedarkone Sep 30, 2013

Sorry, it's just that I've already been through a couple of discussions like this one 😅. The gist I'm linking to is 3 years old after all 😛.

Sorry, it's just that I've already been through a couple of discussions like this one 😅. The gist I'm linking to is 3 years old after all 😛.

@ernie

This comment has been minimized.

Show comment
Hide comment
@ernie

ernie Sep 30, 2013

Collaborator

Likewise. Didn't mean to be dismissive, it's just that the vast majority of folks in Ruby are concerned with a much less severe situation than you describe when reporting this type of thing. Also, I am severely jetlagged and grumpy. :p

Collaborator

ernie commented Sep 30, 2013

Likewise. Didn't mean to be dismissive, it's just that the vast majority of folks in Ruby are concerned with a much less severe situation than you describe when reporting this type of thing. Also, I am severely jetlagged and grumpy. :p

@ernie

This comment has been minimized.

Show comment
Hide comment
@ernie

ernie Sep 30, 2013

Collaborator

So, I ran the code in the gist, and also replaced it with some code that uses the Hash block syntax to set the values, as well. Both appear to work as I described in MRI, which shows how MRI-centric my thinking tends to be.

With Rubinius, I got an error about an index being out of bounds, and with JRuby, processor usage remained pegged but the process never finished.

Updating the gist to use ThreadSafe::Cache did fix the issue on JRuby, but in Rubinius (2.0.0rc1) I still get errors after running for a bit. Backtrace is as follows:

An exception occurred running ./thread.rb
    undefined method `get' on ThreadSafe::Util::XorShiftRandom (Module) (NoMethodError)

Backtrace:
   Kernel(Module)#get (method_missing) at kernel/delta/kernel.rb:81
  ThreadSafe::AtomicReferenceCacheBackend::Node#try_await_lock at \
          .gem/rbx/1.8.7/gems/thread_safe-0.1.3/lib/thread_safe/atomic_reference_cache_backend.rb:276
  ThreadSafe::AtomicReferenceCacheBackend(ThreadSafe::Cache)#try_await_lock at \
          .gem/rbx/1.8.7/gems/thread_safe-0.1.3/lib/thread_safe/atomic_reference_cache_backend.rb:759
  ThreadSafe::AtomicReferenceCacheBackend(ThreadSafe::Cache)#get_and_set at \
          .gem/rbx/1.8.7/gems/thread_safe-0.1.3/lib/thread_safe/atomic_reference_cache_backend.rb:485
  ThreadSafe::AtomicReferenceCacheBackend(ThreadSafe::Cache)#[]= at \
          .gem/rbx/1.8.7/gems/thread_safe-0.1.3/lib/thread_safe/atomic_reference_cache_backend.rb:404
              { } in Object#__script__ at thread.rb:7
                 ThreadSafe::Cache#[] at .gem/rbx/1.8.7/gems/thread_safe-0.1.3
                                         /lib/thread_safe/cache.rb:38
              { } in Object#__script__ at thread.rb:11
                 Integer(Fixnum)#times at kernel/common/integer.rb:83
              { } in Object#__script__ at thread.rb:11
                        Thread#__run__ at kernel/bootstrap/thread18.rb:56
Collaborator

ernie commented Sep 30, 2013

So, I ran the code in the gist, and also replaced it with some code that uses the Hash block syntax to set the values, as well. Both appear to work as I described in MRI, which shows how MRI-centric my thinking tends to be.

With Rubinius, I got an error about an index being out of bounds, and with JRuby, processor usage remained pegged but the process never finished.

Updating the gist to use ThreadSafe::Cache did fix the issue on JRuby, but in Rubinius (2.0.0rc1) I still get errors after running for a bit. Backtrace is as follows:

An exception occurred running ./thread.rb
    undefined method `get' on ThreadSafe::Util::XorShiftRandom (Module) (NoMethodError)

Backtrace:
   Kernel(Module)#get (method_missing) at kernel/delta/kernel.rb:81
  ThreadSafe::AtomicReferenceCacheBackend::Node#try_await_lock at \
          .gem/rbx/1.8.7/gems/thread_safe-0.1.3/lib/thread_safe/atomic_reference_cache_backend.rb:276
  ThreadSafe::AtomicReferenceCacheBackend(ThreadSafe::Cache)#try_await_lock at \
          .gem/rbx/1.8.7/gems/thread_safe-0.1.3/lib/thread_safe/atomic_reference_cache_backend.rb:759
  ThreadSafe::AtomicReferenceCacheBackend(ThreadSafe::Cache)#get_and_set at \
          .gem/rbx/1.8.7/gems/thread_safe-0.1.3/lib/thread_safe/atomic_reference_cache_backend.rb:485
  ThreadSafe::AtomicReferenceCacheBackend(ThreadSafe::Cache)#[]= at \
          .gem/rbx/1.8.7/gems/thread_safe-0.1.3/lib/thread_safe/atomic_reference_cache_backend.rb:404
              { } in Object#__script__ at thread.rb:7
                 ThreadSafe::Cache#[] at .gem/rbx/1.8.7/gems/thread_safe-0.1.3
                                         /lib/thread_safe/cache.rb:38
              { } in Object#__script__ at thread.rb:11
                 Integer(Fixnum)#times at kernel/common/integer.rb:83
              { } in Object#__script__ at thread.rb:11
                        Thread#__run__ at kernel/bootstrap/thread18.rb:56
@ernie

This comment has been minimized.

Show comment
Hide comment
@ernie

ernie Sep 30, 2013

Collaborator

Update: because threading is fun, a second run under Rubinius completed without raising the above error.

Collaborator

ernie commented Sep 30, 2013

Update: because threading is fun, a second run under Rubinius completed without raising the above error.

@thedarkone

This comment has been minimized.

Show comment
Hide comment
@thedarkone

thedarkone Sep 30, 2013

That Rubinius error is a concurrent autoload error, I thought they fixed it in rubinius/rubinius#2080.

That Rubinius error is a concurrent autoload error, I thought they fixed it in rubinius/rubinius#2080.

@thedarkone

This comment has been minimized.

Show comment
Hide comment
@thedarkone

thedarkone Sep 30, 2013

@ernie Rubinius is at RC1 for about 11 months now, can you check that your version has this commit rubinius/rubinius@1c0ccd9 from about 5 months ago? I can't repro that error with my build of rbi (not that this would be indicative of anything when it comes to threading).

@ernie Rubinius is at RC1 for about 11 months now, can you check that your version has this commit rubinius/rubinius@1c0ccd9 from about 5 months ago? I can't repro that error with my build of rbi (not that this would be indicative of anything when it comes to threading).

@ernie

This comment has been minimized.

Show comment
Hide comment
@ernie

ernie Sep 30, 2013

Collaborator

@thedarkone That was the actual RC1 release, I believe, as installed via ruby-install. I have been attempting to get the current dev version to build but not having any luck. :/

Collaborator

ernie commented Sep 30, 2013

@thedarkone That was the actual RC1 release, I believe, as installed via ruby-install. I have been attempting to get the current dev version to build but not having any luck. :/

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 10, 2013

Member

@thedarkone could you rebase the PR?

cc @tenderlove

Member

rafaelfranca commented Nov 10, 2013

@thedarkone could you rebase the PR?

cc @tenderlove

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Nov 10, 2013

Member

Can we precalculate the dispatch cache and make it read only? Then we don't need a threadsafe hash.

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Nov 9, 2013, at 8:26 PM, Rafael Mendonça França notifications@github.com wrote:

@thedarkone could you rebase the PR?

cc @tenderlove


Reply to this email directly or view it on GitHub.

Member

tenderlove commented Nov 10, 2013

Can we precalculate the dispatch cache and make it read only? Then we don't need a threadsafe hash.

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Nov 9, 2013, at 8:26 PM, Rafael Mendonça França notifications@github.com wrote:

@thedarkone could you rebase the PR?

cc @tenderlove


Reply to this email directly or view it on GitHub.

@brixen

This comment has been minimized.

Show comment
Hide comment
@brixen

brixen Sep 8, 2014

Any plans to fix this?

brixen commented Sep 8, 2014

Any plans to fix this?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 9, 2014

The patch looks good to me. If the addition of a dependency is not a problem, I'd say go for it. If, however, the dependency is a sticking point...or if @tenderlove is right and this could be calculated once and store immutably, there are some other options...

In the short term, to avoid adding another dependency, pre-calculating the cache and saving an immutable copy is probably the least invasive option.

In the long term, we can either incorporate thread_safe and use TS::Cache in a future release, or better yet we could remove the atomic gem dependency, add a dependency on concurrent-ruby (which contains or will contain all these tools).

In any case, it would be very nice to have this fixed in the next release :-)

headius commented Sep 9, 2014

The patch looks good to me. If the addition of a dependency is not a problem, I'd say go for it. If, however, the dependency is a sticking point...or if @tenderlove is right and this could be calculated once and store immutably, there are some other options...

In the short term, to avoid adding another dependency, pre-calculating the cache and saving an immutable copy is probably the least invasive option.

In the long term, we can either incorporate thread_safe and use TS::Cache in a future release, or better yet we could remove the atomic gem dependency, add a dependency on concurrent-ruby (which contains or will contain all these tools).

In any case, it would be very nice to have this fixed in the next release :-)

@GeekOnCoffee

This comment has been minimized.

Show comment
Hide comment
@GeekOnCoffee

GeekOnCoffee Sep 9, 2014

thread_safe is a dependency of activesupport, which may or may not making pulling it in here less of a concern.

I imagine nothing will be done with it without the build passing and master merged in so that it'll merge cleanly...

thread_safe is a dependency of activesupport, which may or may not making pulling it in here less of a concern.

I imagine nothing will be done with it without the build passing and master merged in so that it'll merge cleanly...

- @quoted_tables = {}
- @quoted_columns = {}
+ @quoted_tables = ThreadSafe::Cache.new
+ @quoted_columns = ThreadSafe::Cache.new

This comment has been minimized.

@tenderlove

tenderlove Sep 9, 2014

Member

Visitor instances aren't shared among threads, so I don't think we don't need these. (Side note: I'm not sure that those caches are useful anyway)

@tenderlove

tenderlove Sep 9, 2014

Member

Visitor instances aren't shared among threads, so I don't think we don't need these. (Side note: I'm not sure that those caches are useful anyway)

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 9, 2014

@GeekOnCoffee Ahh that certainly seems like it would lessen the pain :-)

headius commented Sep 9, 2014

@GeekOnCoffee Ahh that certainly seems like it would lessen the pain :-)

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Sep 22, 2014

Member

This should be fixed in b57a11c

Member

tenderlove commented Sep 22, 2014

This should be fixed in b57a11c

@tenderlove tenderlove closed this Sep 22, 2014

@brixen

This comment has been minimized.

Show comment
Hide comment
@brixen

brixen Sep 22, 2014

Thanks @tenderlove!

brixen commented Sep 22, 2014

Thanks @tenderlove!

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