search

elia / dimensions-rails

Improve browser rendering adding the size to <img> tags
http://elia.github.com/dimensions-rails/
MIT License
49 stars 10 forks source link

Dimension lookup caching #14

Open nnc opened 10 years ago

nnc commented 10 years ago

Are there any plans to cache the lookups either on first hit for a file, or for all files on startup?

I've measured it quickly on a page with 15 images and it adds 5ms of overhead on my local idle and beefy machine, and I image it could easily be even more on a relatively busy production server. If disk i/o is already high or slow, things would be even worse.

It looks like this calls for a layer of caching so we can skip the filesystem activity and speed things up a bit. Have you given any thoughts to this? Am I overlooking something?

Thanks for this nice little gem!

elia commented 10 years ago

Sorry for the delay

Great idea, I guess it would be a good choice to cache the lookup, probably using Rails.cache. I'd be happy to merge a PR for this :blush:

nnc commented 10 years ago

I've looked into this now, but it seems like Rails.cache is not going to be so straight forward because it defaults to FileStore, which will likely make things even slower. Also, there's expiration issues to think about because we need to take into account non asset pipeline images, whose filename remains the same even after the image is changed, so we can't just cache those forever.

I think we have two options here:

  1. Use an instance variable, so its automatically cleared on app restart. But thats a bit sneaky maybe, and if theres a lot of images that are actually being accessed, memory usage could spiral out of control quickly.
  2. Use Rails.cache, but create a separate MemoryStore in case when FileStore is used, so anyone who configured MemCacheStore or anything other then FileStore would still be using the cache of their choice, and we handle the default case more appropriately. Also, we need to hook into app startup and clear our cache because of those non-asset pipeline images. This is obviously not important for MemoryStore, but for MemCacheStore and others it is, so we have to do it anyway.

And in any case, it would be nice to provide an option to opt out of caching.

And suddenly this cute little gem is not so little any more. Not really sure what would be the best thing to do, if anything.

Care to share your thoughts on the matter?

elia commented 10 years ago

Ok, let the cache be opt-in then, at app configuration level, something like config.dimensions_rails_cache = my_cache (e.g. could be {} for a dummy app level thread-unsafe memoization)

and by default we bake in a fake cache that responds to just #fetch(key,&block)

makes sense?