Use regular cache table for features_codecache

So I was wondering why you want this out of the variables table and in it's own table or the cache table? The reason it is in the variables table is b/c the data is considerably longer living than the transient cache table and it is a kind of expensive operation to regenerate all of those hashes for the code to see if it overridden, etc. It really is not that much data.

Gotcha, ok. Will review soon.

any progress on this?

I would like to store feature content in cache so that I can then put it in memcache and speed up the page loading.

We are waiting for a patch (to add CACHE_PERMANENT?) and more testing on this.

Moving the cache won't necessarily speed anything up. variable_get is extremely fast since it's grabbing the value from the $conf variable in memory already loaded from variables table. So this patch could actually slow things down. However, it *would* reduce the memory usage I think.

But playing with the cache is something that needs more work and somebody to take on this issue and try to do it right.

@mpotter CACHE_PERMANENT is the default. The patch is already using that. Though from the docs:

CACHE_PERMANENT: Indicates that the item should never be removed unless explicitly told to using cache_clear_all() with a cache ID.

@febbraro What do you think on the matter?

RE @febbraro

It really is not that much data.

[warning]
WD memcache: Spent 40.12 ms splitting 1.03 MB object into 2 pieces, cid = em_-cache-features_module_info

It seems more than memcache can handle being in the cache table.

Whoops, features_module_info is a different problem, spoke too soon.

Well opened up a related then... #2421127: Features should have it's own cache bin because it's taking up large hunks > 1M and memcache is failing

Been using #1 for some time now and it's worked well. Thanks @scor

This needs to be bumped and potentially re-submitted. It needs to pass the testing-bot before it can be accepted.

How do we get testbot to actually run?

If this doesn't cause the test bot to run then somebody should re-submit the patch.

Re-submitting @scor's patch from #1

Back to RTBC, since I didn't change code.

joelpittet, please stop messing with the issue status. Don't set it to RTBC until it has passed the testbot. That is causing it to get confused. I'm going to leave the status alone for an hour or so and see if the testbot can complete the request. Otherwise, let me toggle the status. The testbot normally fires when an issue status changes from "needs work" to "needs review"

@mpotter Very strange, I was under the impression that RTBC(from work on D8 core) was not only going to run testbot, but in core it is even re-tested once in a while to see if a committed patch before it makes it no-longer apply.

That may only be for D8 core though I found the meta:
https://www.drupal.org/node/1952058

Sweet it actually failed! Testbot you're alive! Likely needs a cache reset, I'll take a look.

@mpotter I think it may be nicer patch if we could piggy back this on the same new cache table introduced by #2421127: Features should have it's own cache bin because it's taking up large hunks > 1M and memcache is failing

Here's my take on resolving the cache reset, and a bit surprised it didn't fail with variable_get/set the way it's written now.

Hmm, well I guess I need to step through that test and see what's up.

Hi guys,

I've just noticed this issue on my site and was wondering if there is something I can do to help with resolving it. It seems like a waste of server resources on so many sites that are using Features. Hope we can get it resolved fast.

@temkin it would be great if you could help. Likely the fastest is to step through the 3 failing tests with a debugger and find out what is going wrong.

I started to look into that but haven't had much luck so far. Unfortunately even latest Features release without any modifications fails tests in my local environment. I'm getting this error. Any idea why it's happening? I'm willing to look forward but don't know how to resolve that issue. Any assistance is welcomed.

Here is a patch that worked for me and passed auto-tests. Appeared to be just a code cleanup.

Although it took me some time to figure out why tests were failing on a clean Features install. Features Test module has a dependency on CTools, Views and Strongarm. Without those modules tests will always fail. But that's a separate issue.

@temkin an interdiff would be nice there to help people following see what has changed but I manually did my own and noticed you dropped the check for !$reset, was this intentional?

if (!$reset && $cache = cache_get('features_codecache') && !empty($cache->data)) {

to

     $cache = cache_get('features_codecache');
     if (!empty($cache->data)) {

Yes. I don't know what logic is tied to 'reset' therefore decided not to introduce that change in my commit. It's not there in a latest version of code.

I think #32 looks pretty good, but I'd like to see more people try it on real sites.

The $reset should not be used in the 'cache' option of get_signature, so #32 is correct and #24 is wrong. You'll see that set_signature is calling get_signature with the $reset to TRUE to rebuild the signature data. The 'cache' option of get_signature should always just return the cache and not try to rebuild it. So I think that's why the automated tests were failing (which was good)

@temkin @mpotter thanks I must have thought so because the others do that and it's a very common pattern to have a $reset variable in a function param to bypass getting from cache. Maybe that parameter just needs documentation to be explicit why it's wrong.

Having trouble with the patch in #32. It's causing some known overrides to not show in the "drush fl" list. For example, I have a strongarm variable in a feature. If I change it's value via "drush vset" then do a "drush fl", normally the feature is shown as overridden. With the patch in #32 it doesn't show as "overridden" even though "drush fd" still shows the difference.

So somehow this is caching the signatures and this cache isn't getting cleared or set properly. Will look into it.

Here is a new patch for review that I think fixes the issues with signature caching. I'd love some help testing this so we can get it in soon.

Those test failures are they due to the patch or in head?

Reproduced the test failures locally, so debugging now. Seems that the extra "fix" in #39 has some side-effects.

So the problem with all of this is that none of this caching is permanent. Storing in variables made this data persistent...the only time the variable was empty was on initial site install. Cache tables are cleared by drupal_flush_all_caches(), so now the data is reset on every "drush cc all", making Features think it's installing for the first time (where the REBUILDABLE state is distinct from the OVERRIDE state). This causes some modules that should be marked as "overridden" to be marked as "rebuildable" instead (which isn't shown in drush fl).

The only real fix I can see for this is to create yet another cache table. We can't re-use the table created in #2421127 because that table needs to be cleared on cache clear.

Basically, this data is *state* data and not *cache* data. Which is why it was stored in the variables to begin with.

OK, here is a patch that adds another cache table that is *not* marked to be cleared by drupal_flush_all_caches().

Woot, there we go! OK, tempted to RTBC this, but maybe somebody else can give a quick review?

Well that changes less.

OK, committed to 1793a57.

I still see this issue in the latest version of Features. Somehow it wasn't committed?

Ah nice catch, the patch still applies, back to RTBC. @mpotter you shouldn't need to mention the commit hash if the commit message is used from the generated one below on the issue unless it was committed in some other issue or a mistake?

Still using this, still +1

Thanks @joseph.olstad

Hello!
Can somebody explain why this was done?
Was there a misbehavior that is now fixed? Or do we want to un-pollute the variables?

What is the purpose of this signature/codecache?
(I am answering myself below)

(from #3)

It's for consistency. the variable table is not a caching table.

What makes this a "cache" value?

Only the name, "codecache", suggests that it is a cache.

The API tells a different story:
features_get_signature() and features_get_signature(), this is the API for a persistent value or state.

features_get_signature('normal') gets an md5 signature reflecting the component state in the database.
features_get_signature('default') gets an md5 signature reflecting the component state in code.
features_get_signature('cache') gets the stored md5 signature for 'default' (= code) from some time in the past, when the user did a specific operation.

Unlike a real cached value, the "cached" signature cannot be re-calculated from other values at any time.
if it is, we lose the information whether a feature needs review.

Therefore it does not semantically belong into a cache table.

Perhaps this does not matter if we never clear this cache.
But keep in mind that cache tables are also typically not part of a db dump.
So there are ways that this data can be lost, if we put it into a cache table.

Perhaps the 'variables' table is also not the best place for it. But perhaps it is good enough.
Yes, this is loaded into memory on every request. But the data is comparably small.

An alternative would be a dedicated table.
If we do this, we could have one record per features component.
Perhaps this would be faster? Not sure.

EDIT: I am learning about this system while writing and editing this comment :)

Or do we want to un-pollute the variables?

@joelpittet mentioned to me that this was indeed about the size of the variable.
https://twitter.com/joelpittet/status/1289951267695947778

I agree with this motivation: The variable table was not the right place for this.

The question remains, is a cache table the correct place for it?
The table name is 'cache_codecache', so I think this will be excluded in database dumps if a "cache_" prefix is used to determine which data to exclude.
Semantically it is not really a cache.

I would suggest a new dedicated table 'features_signatures', where either each row is one component in one feature module, or where the components for one feature are stored together in one row, serialized. I think the former is cleaner.

Created a follow-up:
#3162854: Move 'features_codecache' / 'cache_featurestate' to a dedicated db table 'features_signatures'