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

Spec errors in new optimizations #1

Closed
gdeglin opened this issue Dec 12, 2014 · 5 comments
Closed

Spec errors in new optimizations #1

gdeglin opened this issue Dec 12, 2014 · 5 comments

Comments

@gdeglin
Copy link

gdeglin commented Dec 12, 2014

The new optimizations look great, thanks!

I ran into a few errors from a fresh clone of the new code. Just in case you're not already aware of these:

  1) Modis::Attribute :timestamp type is coerced
     Failure/Error: expect(found.created_at.to_s).to eq(str_time)

       expected: "2014-12-11 17:31:50 -0200"
            got: "2014-12-11 11:31:50 -0800"

       (compared using ==)
     # ./spec/attribute_spec.rb:102:in `block (3 levels) in <top (required)>'

  2) Modis::Attribute :hash type is coerced
     Failure/Error: expect(found.hash).to eq(foo: :bar)

       expected: {:foo=>:bar}
            got: {"foo"=>"bar"}

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -:foo => :bar,
       +"foo" => "bar",
     # ./spec/attribute_spec.rb:149:in `block (3 levels) in <top (required)>'

  3) Modis::Attribute variable type is coerced
     Failure/Error: expect(found.string_or_hash).to eq(foo: :bar)

       expected: {:foo=>:bar}
            got: {"foo"=>"bar"}

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -:foo => :bar,
       +"foo" => "bar",
     # ./spec/attribute_spec.rb:162:in `block (3 levels) in <top (required)>'
@ileitch
Copy link
Member

ileitch commented Dec 12, 2014

Yeah, I wasn't in the mood to fix them yet :) I'm still working on more optimizations, I've only profiled find and where so far.

@ileitch
Copy link
Member

ileitch commented Dec 12, 2014

P.S Are you using Rpush at GameThrive? If so, that's awesome!

@gdeglin
Copy link
Author

gdeglin commented Dec 12, 2014

Yep, we are. Building on top of rpush saved us a bunch of time. Thanks so much! :)

We made some performance optimizations to modis in our fork that you might want to use as well:

  1. Modifying the persist method to only save attributes that are different than the defaults. The initialize method inside of model.rb automatically populates default values before assigning attributes so we didn't even have to modify that part at all. This saves us about 50% of redis memory usage and speeds things up a lot.
  2. Adding a find_multiple finder method (Basically find, but accepts an array). We then wrap all the calls to record_for inside of a redis.pipelined block. We use this method in our version of rpush and it speeds things up a lot.
  3. Using pipelining around some of the things that happen in persist.

@ileitch
Copy link
Member

ileitch commented Dec 14, 2014

I've now implemented 1 & 2 (find takes multiple IDs, like ActiveRecord).

Can you point me at the changes you made to make further use of pipelining?

@gdeglin
Copy link
Author

gdeglin commented Dec 15, 2014

Yeah. I basically moved redis.sadd(self.class.key_for(:all), id) right under redis.hmset(self.class.key_for(id), attrs) instead of being in the track() callback. Then I wrapped both of those calls inside redis.pipelined.

It's possible add_to_index/update_index could be pipelined as part of this as well. In my case I just ended up removing the index related code since it's not used by rpush.

@ileitch ileitch closed this as completed Jun 27, 2015
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

No branches or pull requests

2 participants