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

Implement fetch_multi for parity with current Rails #380

Merged
merged 1 commit into from Jul 19, 2013

Conversation

Projects
None yet
4 participants
@sorentwo
Copy link
Contributor

sorentwo commented Jul 12, 2013

As of Rails 4.1.X the base ActiveSupport::Cache implementation has a fetch_mutli method. This simply ports that method over into the DalliStore.

There is room for improvement in writing out the keys, possibly by pipelining them within a multi block.

@mperham

This comment has been minimized.

Copy link
Collaborator

mperham commented Jul 12, 2013

Does this handle long keys (greater than 255 chars) correctly?

@sorentwo

This comment has been minimized.

Copy link
Contributor

sorentwo commented Jul 12, 2013

I just checked into that by changing the implementation of rand_key to

def rand_key
  SecureRandom.hex(129)
end

And it looks like neither read_multi or fetch_multi, which relies on it, support keys over 255 characters. Is this a known shortcoming of read_multi? Something that needs to be fixed in general?

hash = { x => 'ABC', y => 'DEF' }

@dalli.write(y, '123')
@dalli.fetch_multi(x, y) { |key| hash[key] }

This comment has been minimized.

@mperham

mperham Jul 17, 2013

Collaborator

You should assert the return value of fetch_multi.

@joerixaop

This comment has been minimized.

Copy link
Collaborator

joerixaop commented Jul 17, 2013

Wouldn't it make more sense to use get_multi and then check which items are missing in the reply and set only those?
That way you get pipelining for free

@sorentwo

This comment has been minimized.

Copy link
Contributor

sorentwo commented Jul 17, 2013

@mperham I've added an assertion on output here. Doing so was pretty crucial because it exposed two major issues:

  1. The results were an array, and not a hash. That is what I had been using with my monkey patched version locally but it wasn't consistent with read_multi.
  2. The result was not identity mapped, it fell down when you gave it integer based keys instead of strings.

In making these changes I explored performing all reads/writes within a single multi block (d6909b4) and benchmarked it here. The second implementation (8c5936c) using get_multi wins handily:

# All read/write operations wrapped inside of a single multi block

Calculating -------------------------------------
               fetch        19 i/100ms
         fetch_multi        33 i/100ms
-------------------------------------------------
               fetch      195.8 (±1.0%) i/s -        988 in   5.045953s
         fetch_multi      335.3 (±0.9%) i/s -       1683 in   5.020209s

# Two separate multi blocks are used, the first leveraging `get_multi`

Calculating -------------------------------------
               fetch        19 i/100ms
         fetch_multi        78 i/100ms
-------------------------------------------------
               fetch      192.2 (±2.1%) i/s -        969 in   5.044695s
         fetch_multi      759.7 (±2.5%) i/s -       3822 in   5.034430s
@sorentwo

This comment has been minimized.

Copy link
Contributor

sorentwo commented Jul 17, 2013

I just dug up the original pull request for ActiveSupport::Cache's implementation of fetch_multi: rails/rails#10234

There isn't any discussion over returning an array vs a hash, so I'm wondering if consistency should be a goal. In my use case (and I imagine many others) I only need the values and have no need for the hash. Thoughts on this?

@mperham

This comment has been minimized.

Copy link
Collaborator

mperham commented Jul 17, 2013

I would prefer a Hash. That's consistent with read_multi and I can sense a lot of use cases where the user would otherwise need to use Enumerable#zip to iterate through the keys/values in pairs.

@sorentwo

This comment has been minimized.

Copy link
Contributor

sorentwo commented Jul 19, 2013

@mperham Is there anything else I can do to help this get pulled in?

@mperham

This comment has been minimized.

Copy link
Collaborator

mperham commented Jul 19, 2013

Looks good. Update the History with a new feature note and to give yourself credit and I'll merge.

Implement fetch_multi for parity with current Rails
As of Rails 4.1.X the base ActiveSupport::Cache implementation has a
`fetch_mutli` method. This ports that method over into the dalli_store.
Reading and writing are pipelined within two separate blocks for
efficiency.
@sorentwo

This comment has been minimized.

Copy link
Contributor

sorentwo commented Jul 19, 2013

Added the history note and squashed them up.

mperham added a commit that referenced this pull request Jul 19, 2013

Merge pull request #380 from sorentwo/fetch-multi
Implement fetch_multi for parity with current Rails

@mperham mperham merged commit 7c0a0ca into petergoldstein:master Jul 19, 2013

1 check was pending

default The Travis CI build is in progress
Details
@mperham

This comment has been minimized.

Copy link
Collaborator

mperham commented Jul 19, 2013

Real nice work. Thanks!

@maxguzenski

This comment has been minimized.

Copy link

maxguzenski commented Aug 20, 2013

Can I use this feature with rails 3.2, ruby 2 and unicorn?

@sorentwo

This comment has been minimized.

Copy link
Contributor

sorentwo commented Aug 20, 2013

@maxguzenski I'm using it with Rails 3.2 on Ruby 2.0, and Unicorn would have no bearing on its usage. There isn't a gem release though, so you'll need to pull it in via git.

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