Use a dedicated cache table

I'm agree, I think that it's necesary. I attach a little change to avoid duplicated code.

Apologies for my ignorance on this, I don't really know how Drupal's caching system works. What exactly does this patch do? And why is it needed?

I don't understand why #1 entails "s3fs records are gone". The s3fs_file table is the only "record" that S3FS keeps, and it's not erased by a cache clear. And in step #2, what does "read_cache doesn't find the files" mean? I seems to be missing an important chunk of information about Drupal's internal operations.

Hi coredumperror,

Implementing a Drupal custom cache table means that you can choose if the cache is permanent or not and other things more like use a separate cache table.

pcambra say that use a separate table cache to avoid this data was cleared, because if we don't implement hook_flush_caches to clear our new cache, cache will be permanent, and the performance don't be affected when we cleared general cache with "drush cc all" for example.

s3fs_file table will reduce queries instead use cache because the cache won't lose the data.

In other hand, lock functions like lock_acquire, lock_wait or lock_release that do more database queries will be reduced too.

This improvement is more if you use a cache like memcached because you never do database queries for that unless you restart your memcached service.

pcambra complete if you think.

P.D. coredumperror thanks a lot for the mantainer.

OK, I think I understand now what the patch does and why you'd want to add it. But I still don't really get why caching even comes into play. Why does s3fs care about what Drupal is putting in the cache? How does that affect s3fs's operation?

S3fsStreamWrapper class use this cache in _read_cache method. This method appears 8 times in this class more or less.

Ohhhh, I completely forgot that S3fsStreamWrapper::_read_cache() used Drupal's cache for a performance boost. Now I get what this proposed change is all about.

However, unless I miss my guess, it's kindof a pointless change. The only reason that S3fsStreamWrapper::_read_cache() uses caching is for those sites which have a non-database caching backend (e.g. redis, memcache) installed. Creating a table in the DB for this cache is pointless, because it'll just end up being a copy of the s3fs_file table.

Admittedly, this means that sites which don't have a high performance caching backend installed shouldn't be caching these reads at all. All that does is duplicate a bunch of data in the DB and provide no performance improvement. That's a failure on my part, so I'm going to fix it.

I'll write a patch that only enables caching of s3fs_file table reads if a high-performance caching backend is installed.

OK, here's a patch I came up with for this issue, as described in my last comment. I'm only ~80% sure this makes sense, though, since I'm obviously not an expert on caching. Could you guys take a look at the patch and let me know if it makes any sense to do it like this?

What this patch does is skip all the caching code unless a high performance cache module (memcache or redis) is installed. It also adds an INFO line to the Status Report page (/admin/reports/status) to let users know when it will be caching its DB reads. I ended up having to add the cache table code from #3 because Drupal's caching system expects the table to exist, even though that table in the DB will never actually be used.

Hi guys, I prefer #2 with #3. I attach a queries with only one file by diferent configurations.

I think that is useful use s3fs bin cache for database cache too. We will save four queries using it in each _read_cache() call, since the second use until "ever".

Regarding the chart in #11, I think you're basically saying that my patch in #9 is actually the best solution. It also implements the s3fs cache bin from #2, but only does any caching at all if memcache or redis is installed.

I tried to make it the "best of both worlds". It avoids pointlessly writing to the cache if it's implemented in the database, but does use a high-performance cache if one is available.

Sorry I miss a few lines in plain text mode.

I understand now globally the code, only we must change

cache_set($cid, $record, S3FS_CACHE_BIN, CACHE_TEMPORARY);

for

cache_set($cid, $record, S3FS_CACHE_BIN);

To get permanent cache.

Other things to be thought avoiding cache for all cases and only use for high performance cache:
- module_exists doesn't implie a properly configuration for cache modules to external caches works or only be used instead drupal database cache.
- if new cache system is used out of memcache or redis, we have to do new change. While using cache system like #2, #3 and this cache_set changes, we don't have to do nothing anymore, because drupal will manage what cache will be used and use that cache implementation.

Edit: Maybe It worths lose 4 queries in the first time access for be compatible with all cache implementations.

- module_exists doesn't implie a properly configuration for cache modules to external caches works or only be used instead drupal database cache.

Yeah, I know. But what else can s3fs do? You could potentially read up on the internal workings of the memcache and redis modules, to try to learn what "valid" configuration settings are, I guess. But that seems like overkill, not to mention that it would couple s3fs to those modules much more tightly than I'd like. Assuming that someone who has the appropriate module installed and enabled also has it configured correctly seems like the most logical solution. module_exists() is also extremely fast compared to querying the database for another module's config settings.

- if new cache system is used out of memcache or redis, we have to do new change. While using cache system like #2, #3 and this cache_set changes, we don't have to do nothing anymore, because drupal will manage what cache will be used and use that cache implementation.

True. But if we let Drupal manage it for us, and don't do the memcache/redis check, then we're back to square one when a high performance cache isn't installed: duplicating the contents of the s3fs_file table into the s3fs_cache table, for no good reason. I'll be honest... it's probably overkill to cache these reads at all, since they're extremely simple SELECT queries. I only added the caching code in the first place because memcache/redis are so much faster than the DB, and s3fs could use all the performance benefits it can get.

Hi man,

duplicating the contents of the s3fs_file table into the s3fs_cache table, for no good reason

I'm agree, is the only thing that I don't like for #2, #3 solution although now this duplicated rows are successing, I like your solution only say some things to keep in mind.

I have been thinking about s3fs_detect_high_performance_cache() to improve with all external database. We can check with this:

return variable_get('cache_default_class', 'DrupalDatabaseCache') !== 'DrupalDatabaseCache';

What do you think?

Calling variable_get() would negate the entire benefit of caching the s3fs_file queries, since it also queries the database.

I'm starting to realize that caching these DB reads at all is pretty much pointless. It's not like the reads we're caching are remotely complex or time-consuming; they're just basic SELECTs on a single table. The tiny performance benefit of caching those reads in a memcache or redis server just doesn't seem to be worth the headache of detecting whether such a cache mechanism is in place. Hell, it may not even be a performance benefit at all, due to those semaphore locking calls.

I've attached a patch that just removes the caching entirely. What do you think?

Hi!

Calling variable_get() would negate the entire benefit of caching the s3fs_file queries, since it also queries the database.

But if we use static or drupal static like in your function (s3fs_detect_high_performance_cache), it'll be only one query for all the request.
I see cache usefull in this case, sites like press sites or other high performance sites often have a lot of images, use s3 buckets add more steps between the backend and the files, I think if we can save something, do it.

In other hand, I have to read more about lock system, maybe we can avoid that in our case, now I don't know if is good idea or not.

The thing is that the caching code in question has nothing to do with the long time it takes to make an API call out to the S3 bucket itself. The only thing that's being cached here is a DB query against the s3fs_file table. s3fs does all kinds of other things to avoid API calls to S3 as much as it can, but this change is just removing the cache for that DB query that's already plenty fast.

And yeah, we could try to improve the performance of the variable_get() call you suggested by using static, but that's putting lipstick on a pig. It's a tiny improvement to a system that's already ugly and mostly pointless.

I removed the s3fs_file read cache, as it was pointless (and possibly even detrimental) in the majority of Drupal installs.