Cast TTL values to integers to better support ActiveSupport::Duration values.

[chriseppstein]

This pull request addresses issue #39 which is still present despite being closed and agreed to be fixed if a pull was submitted.

Cast TTL values to integers to better support ActiveSupport::Duration values. To avoid special casing Active::SupportDuration, this patch just calls to_i on the ttl value, but this causes a problem if the the argument is a Time object. So this patch also now allows Time values to be passed as long as they are in the future by converting them to a duration of time in seconds.

[evan]

Hmm. In #39 I was referring to fixing Interlock, not Memcached. Your change causes a very significant performance degradation:

ttl_fix:
                                              user     system      total        real
    set: libm:ascii                       1.080000   1.700000   2.780000 (  6.599218)
    set: libm:ascii:pipeline              0.520000   0.020000   0.540000 (  0.557255)
    set: libm:ascii:udp                   1.110000   1.270000   2.380000 (  5.586330)
    set: libm:bin                         0.880000   1.740000   2.620000 (  6.603217)
    set: libm:bin:buffer                  0.420000   0.480000   0.900000 (  1.325392)

master:
                                              user     system      total        real
    set: libm:ascii                       0.890000   1.570000   2.460000 (  5.939042)
    set: libm:ascii:pipeline              0.380000   0.020000   0.400000 (  0.392979)
    set: libm:ascii:udp                   0.550000   0.810000   1.360000 (  3.035179)
    set: libm:bin                         0.620000   1.530000   2.150000 (  5.555898)
    set: libm:bin:buffer                  0.320000   0.560000   0.880000 (  1.319815)

Unfortunately I am not going to be able to accept any change to this effect, even just adding .to_i; it was deliberately removed in order to enforce consistent and optimally performant use by the callers of the interface.

[chriseppstein]

It wasn't clear this was a performance change, but I don't think you should just reject this out of hand, perhaps there is a fix that is more performant but still accomodates rails users (who are clearly not very worried about this level of performance)

[chriseppstein]

What did you use for benchmarking so I can run it?

[evan]

CLIENT=libm rake benchmark

Memcached.gem's primary goal is performance, even for Rails users.

[chriseppstein]

Agreed. Which is why I'm using it. But it seems like there is some middle ground here. c/d?

[evan]

No...I have deliberately never committed a change that regressed performance in any way. That's the only way performance has reached the level it has--given how easy it is to destroy well-behaved Ruby code.

What you should do is update the call sites in your application to cast the time values to Fixnums on startup, and then just reuse the same value when possible. The “I have no idea what the caller might give me, so I'm going to try everything” pattern, although very widespread in Ruby, is antagonist to maintainable and performant code.

[chriseppstein]

I disagree with your assertion that your suggestion is more maintainable. You're forcing a level of micro-optimization here that is beyond the needs for many applications and will only matter for apps that need to perform thousands of memcache operations per second/minute/whatever. It seems you have some religion around saving users a few milliseconds per request, but you can at least provide an API shim so that you do not break existing apps assuming they opt in to backwards compatibility.

[evan]

Some of my users have applications that do millions of memcached operations per second. Under no circumstances are they going to get regressed.

If you wanted to subclass Memcached::Rails into Memcached::Rails::Dirty and add flexible calling support there (there might be things other than TTLs people are interested in), then that would be ok. That's the only way to make it optional without a regression in the existing code.

[chriseppstein]

If you will accept an opt-in patch, I will make one, I do not like maintaining forks.

[evan]

Go for it. Make sure you use the subclass strategy, and have tests. Memcached::Rails and its tests can be your example.

[evan]

We found a way to do this without degrading performance. Coming in Memcached.gem 1.4.0.

[chriseppstein]

I keep looking at this on my todo list and then doing more important things :( Thank you.

[evan]

Released! Let me know if it works for you.