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

Pipelining support #927

Open
casperisfine opened this issue Aug 1, 2022 · 6 comments
Open

Pipelining support #927

casperisfine opened this issue Aug 1, 2022 · 6 comments

Comments

@casperisfine
Copy link
Contributor

So as far as I can tell, Dalli's pipelining support is limited to the #quiet method which prevent to get the commands results.

Use case

In Rails' MemCacheStore I'd like to issue two combined commands on #increment:

def increment(key, amount, expires_in: nil)
  counter = @dalli.incr(key, amount, expires_in, amount)
  @dalli.touch(key, expires_in) if expires_in
  counter
end

The problem is that this double the latency as we now need to full rountrips to the Memcached server. If Dalli had a pipelined method like redis-rb, we could do:

def increment(key, amount, expires_in: nil)
  @dalli.pipelined do
    @dalli.incr(key, amount, expires_in, amount)
    @dalli.touch(key, expires_in) if expires_in
  end.first
end

And only wait for a single roundtrip.

Is there a reason such a method wasn't implemented? And if not would a PR be welcome for it?

@petergoldstein
Copy link
Owner

I think it should be possible, but I'm not entirely sure that implementing a simple pipeline, as in redis-rb, is actually doable with memcached.

My understanding of the redis-rb implementation is that the pipelined call returns an array of responses to each command, in the order in which they were invoked on the pipeline. Essentially the client defers writing commands and reading from the response buffer until the end of the block. Intermediate calls return Futures, which resolve at the end of the block.

This approach, if possible, would be a very good fit for Dalli. It would be largely transparent from the perspective of requests sent to the wire, and the parsing of response data received from the wire. A PR that implemented it would be welcome. And it's not immediately obvious to me why it wouldn't work, unless memcached is doing something odd on the server end.

That all said, it's a little odd that the memcached documentation describes pipeline implementations completely differently, which leads me to suspect that the above clean implementation may not work for some reason.

There are two active protocols in memcached and here's the notes they have on pipelining:

  1. meta - Update of ASCII, to be primary protocol going forward (https://github.com/memcached/memcached/wiki/MetaCommands#pipelining-quiet-mode-with-opaque-or-key)
  2. binary - Previously the primary protocol, now deprecated (https://github.com/memcached/memcached/wiki/BinaryProtocolRevamped#get-get-quietly-get-key-get-key-quietly)

In both cases there's a recommendation to use the quiet functionality for pipelining, potentially in concert with the use of k (which reflects back the key) or O (which reflects back a user-defined nonce). This is pretty complex from the client perspective. quiet mode means that mapping responses to commands is now extremely challenging - it's no longer a predictable, sequential mapping. And to address the response issue we need to change the commands we're sending and how the responses are being parsed. I'm not sure why the documentation would omit a simpler option.

Existing Dalli code also seems to indicate an issue. Both multi and the multi_get in Dalli::Client were implemented before I took over as maintainer, so I don't have a great deal of background here. The former discards responses, and the latter uses the quiet mechanism in tandem with the k flag. Both are restricted in terms of what commands they can process. If a simple pipeline like the one in redis-rb were possible, I'm not sure why this approach would have been taken.

So @casperisfine if you have the time to put together a PR and confirm that it works, that would be welcome.

@casperisfine
Copy link
Contributor Author

My understanding of the redis-rb implementation is that the pipelined call returns an array of responses to each command, in the order in which they were invoked on the pipeline. Essentially the client defers writing commands and reading from the response buffer until the end of the block. Intermediate calls return Futures, which resolve at the end of the block.

Pretty much yes. It's purely a client side feature, the client simply write all the commands, and read all the responses sequentially, e.g (pseudo code).

def call_pipelined(commands)
  commands.each do |command|
    @socket.write(command)
  end
  commands.size.times.map do
    read_one_response(@socket)
  end
end

That alone allows to execute N commands with a single roundtrip.

the memcached documentation describes pipeline implementations completely differently

It indeed seem that Memcache has some server side support for pipelining, I'll dig more as to why, but I suspect it's to allow the server to process the command out of order or concurrently.

suspect that the above clean implementation may not work for some reason.

That would surprise me, but I guess the best way to know is to try.

if you have the time to put together a PR and confirm that it works, that would be welcome.

Thanks! I'll try to find time to explore that and at the very least report my findings. One issue that may come up is pipelining already having a specific meaning in Memcached context, so we may need to find another term, but that's only a worry if it end up working.

Another thing I didn't think of before opening this issue, is that Memcached is generally used with client side distribution (or hashing), so the pipelining would need to be sent to different servers concurrently and then the responses put back in order. That may be why Memcached offer some server side pipelining features while redis doesn't (and why Redis::Distributed never bothered implementing pipelining).

@casperisfine
Copy link
Contributor Author

Seems like the quiet mode is to simplify the client work, and to avoid sending useless NOT_FOUND responses:

You can also do the naive thing and send n pipelined get/getks, but then you could potentially get back a lot of "NOT_FOUND" error code packets

@casperisfine
Copy link
Contributor Author

Ok, so I started looking at implementing this, and I must admit it's a lot more work that I initially envisioned, but more importantly it would require either an heavy refactoring or to duplicate a lot of code.

The main issue is that the protocols implementations are not well suited to delay reading the response. e.g. get is implemented as:

      def get(key, options = nil)
        req = RequestFormatter.standard_request(opkey: :get, key: key)
        write(req)
        response_processor.get(cache_nils: cache_nils?(options))
      end

So to be able to write multiple commands, and then later read multiple responses, all of this would need to be decomposed etc.

It's totally doable, but not sure I'll get enough time to do this soon, and also not sure you'd agree on that much changes.

So I'd rather pause for now, if you still think it's a good idea I might get back to it, but also feel free to close this issue as too much of a change.

@casperisfine
Copy link
Contributor Author

Also for the record my quick experiment branch is there: main...Shopify:dalli:client-pipelining

@petergoldstein
Copy link
Owner

Let me keep it open for now. I have some thoughts on how to do that refactoring, as well as how to deal with the ring. I can't look at it right now, but I may be able to get back to it in a bit. And I do think it's valuable, assuming it works.

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