Remove the option for thread_safe #188

Merged
merged 1 commit into from Mar 13, 2012

Projects

None yet

5 participants

@cyx
cyx commented Mar 12, 2012

I think the @synchronizer is just adding an indirection which isn't useful and
just adds a small performance penalty for 99% of the users doing Redis.connect.

@djanowski
Member

I'm +1, let's hear from @soveran and @pietern.

@soveran
Member
soveran commented Mar 12, 2012

+1

@pietern
Member
pietern commented Mar 12, 2012

Yes, sounds good. If users still want to disable it they can #include a fake Monitor mixin.

@pietern pietern merged commit 578c01f into redis:master Mar 13, 2012
@catwell
catwell commented Mar 13, 2012

So how do you disable thread safety now? Is it as simple as this or am I missing something?

Redis.class_eval do
  def synchronize; yield; end
end

@pietern Can you give an example of your fake MonitorMixin? I am never using threads and monitors degrade performance significantly [EDIT: this is wrong, see below].

@djanowski
Member

You can do that, or subclass Redis.

But clearly – why would you want to do that?

@catwell
catwell commented Mar 13, 2012

@djanowski Performance. I have to benchmark that again but I used to disable thread safety because I had noticed an important decrease in performance due to Monitor.

@djanowski
Member

Check again, both @cyx and I made benchmarks and the impact was negligible.

@catwell
catwell commented Mar 13, 2012

I just did a quick bench and I found that the impact was indeed negligible.

My switch to thread unsafe goes back to February 2011 according to git so maybe things have improved in the meantime or maybe I was doing something wrong back then.

Anyway I will use the thread safe version unless I encounter problems again, in which case I will create a dedicated issue.

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