Problem/Motivation

While working on enhancing the Rules caching #2189645 I came across following scenario:
system_cron() invokes $cache_tables = array_merge(module_invoke_all('flush_caches'), $core); and there's entity_flush_caches() which calls entity_defaults_rebuild(). This entity rebuild, goes through RulesPlugin::save() which calls rules_clear_cache().

And there it is, the whole rules cache is lost, no matter if the items where expired or not. On a site with a lot of rules this can lead to stampeding since the rebuild of the rules cache hasn't locking yet (#2190553: Add locking to cache rebuild to avoid stampeding)

Proposed resolution

Features had a similar problem and introduced a variable to disable rebuilding on cache flushes.
We could reuse this and hook into the features rebuilding process.
This by implementing hook_features_rebuild() and run the entity rebuilding when it is invoked.
In entity_flush_caches() we can do a check for the variable of features. If it's there and disabled rebuild on cache flush it means features is installed will takes care of the rebuilding. If the variable isn't set / rebuild on cache flush is enabled we act as usual and do a rebuild.

Remaining tasks

Reviews needed.

User interface changes

None.

API changes

Introduced EntityDefaultFeaturesController::rebuild().

Comments

fago’s picture

Status: Needs review » Needs work

Re-using the features variable makes sense to me.

+++ b/entity.features.inc
@@ -171,8 +194,26 @@ function entity_features_export_render($module, $data, $export = NULL, $entity_t
+ * Implements hook_features_rebuild().
+ *
+ * Features component callback.
+ */
+function entity_features_rebuild($module = NULL, $entity_type) {

Problem is this is a features component callback only - so when no feature component is in rebuildable state, e.g. you have nothing exported or it's all overridden - it won't be fired.

We'd need a way to kick in independently - so we now we can rebuild when features rebuilds its stuff also. Unfortunately, features does not provide a general hook to react on feature-rebuilding yet, so I guess we'd have to add something like that.

Also, a features-rebuild drush command would make sense if rebuild on cache clear is disabled - but that's another story I guess.

das-peter’s picture

Status: Needs work » Needs review
Related issues: +#1844566: Invoke pre and post hooks when ALL components have been enabled
FileSize
3.34 KB
PASSED: [[SimpleTest]]: [MySQL] 630 pass(es). View

Updated approach after stumbling over #1844566: Invoke pre and post hooks when ALL components have been enabled and adding another patch there.
What about this approach? That should make the hook invocation components independent, right?

fago’s picture

Yeah, this approach looks good to me. We'd probably want to add something to the README about it, so people are aware of this option to speed up things :-)

das-peter’s picture

das-peter’s picture

FileSize
4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 630 pass(es). View

We'd probably want to add something to the README about it, so people are aware of this option to speed up things :-)

Attempt on writing something that is understandable - I think I failed though :P

fago’s picture

Status: Needs review » Fixed

Thanks on the great work on this patch and the features patch. I was little worried about people running features 2.0 and missing the new hook, but then I figured features_rebuild_on_flush is quite undocumented - so probably not a lot of people have it activated. I'll make sure to document this in the commit message still though.

Thus, I took your patch and committed it. README text was already good thanks, I tweaked it a little bit more though.

fago’s picture

Status: Fixed » Needs work

uhm, just realized it's not hidden at all - so this could be a problem then. Therefore I did not commit it for now. Attached is an updated patch with my README tweaks.

Let's improve it so it will continue working with features 2.0 and features_rebuild_on_flush set to TRUE also. Maybe best, let's just add an additional flag like entity_rebuild_on_flush = TRUE|FALSE and leave it to the people whether they use features for triggering rebuilds?

das-peter’s picture

Attached is an updated patch with my README tweaks.

I think the patch was lost :)

fago’s picture

FileSize
4.32 KB

ops :)

das-peter’s picture

Well, if we're introducing a new variable anyway - how about making it entity type specific too?
Global: entity_rebuild_on_flush = TRUE|FALSE
Specific: entity_rebuild_on_flush_[entity-type] = TRUE|FALSE

hefox’s picture

fago’s picture

Well, if we're introducing a new variable anyway - how about making it entity type specific too

Why would you disable it per entity-type? Is there a use-case you have in mind?

das-peter’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
5.25 KB
PASSED: [[SimpleTest]]: [MySQL] 630 pass(es). View

Hope I got it right.

SocialNicheGuru’s picture

Will this interfere with features updates or rebuilds using drush?

drush fr feature-name
drush fu feature-name

das-peter’s picture

@SocialNicheGuru No

fago’s picture

Status: Needs review » Needs work
  1. +++ b/entity.features.inc
    @@ -130,6 +136,18 @@ class EntityDefaultFeaturesController {
    +    // entity_defaults_rebuild(array($this->type));
    

    That line should be removed imho, the comment is enough.

  2. +++ b/entity.features.inc
    @@ -171,8 +195,32 @@ function entity_features_export_render($module, $data, $export = NULL, $entity_t
    + * Revert all defaults when a features rebuild is triggered - even the ones not
    

    Rebuild all defaults

  3. +++ b/entity.module
    @@ -1076,7 +1076,10 @@ function entity_flush_caches() {
    +  // Also check if features has explicitly disabled rebuilding on cache flush.
    

    Comment needs an update - its our own setting.

  4. +++ b/views/plugins/entity_views_plugin_row_entity_view.inc
    @@ -88,6 +88,7 @@ class entity_views_plugin_row_entity_view extends views_plugin_row {
         if ($entity = $this->get_value($values)) {
    +      $entity->view = $this->view;
    

    Unrelated.

das-peter’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
4.46 KB
PASSED: [[SimpleTest]]: [MySQL] 630 pass(es). View
  1. Done
  2. Done
  3. Done
  4. Please consider adding it (related issue from September 18, 2014): #2340487: Views Row Plugin "Rendered entity" should add reference to the view.
a.milkovsky’s picture

+  function rebuild($module = NULL) {
+    // We don't do the rebuild here since a whole rebuild is triggered in
+    // entity_features_post_restore().
+  }
+function entity_features_rebuild($module = NULL, $entity_type) {
+  return entity_features_get_controller($entity_type)->rebuild($module);
+}

Why do we need this code if all the logic is done in entity_features_post_restore()?

a.milkovsky’s picture

FileSize
1002 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch innerdiff-2241979-17-19.diff. Unable to apply patch. See the log in the details link for more information. View
3.84 KB
PASSED: [[SimpleTest]]: [MySQL] 630 pass(es). View

#17 looks good for me. Cache clearing is way faster now.
Removed EntityDefaultFeaturesController::rebuild() as all the logic is done in entity_features_post_restore().

The last submitted patch, 19: innerdiff-2241979-17-19.diff, failed testing.

das-peter’s picture

FileSize
3.84 KB
PASSED: [[SimpleTest]]: [MySQL] 630 pass(es). View

@a.milkovsky: Thanks for the update!
Re-uploading patch from #19 without the diff to get it green again.
Please use for interdiffs the file extension .txt or add the suffix do-not-test to avoid unrelated testbot execution / fails.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, this looks ready to me and can go into the next release. Let's put it on the table for others too first.

fago’s picture

Thanks, committed.

fago’s picture

Status: Reviewed & tested by the community » Fixed

  • das-peter authored 0a750f2 on 7.x-1.x
    Issue #2241979 by das-peter, a.milkovsky, fago: Add a way to prevent...
hefox’s picture

So, now on every feature rebuild/revert, alll the entity defaults are rebuilt? That might cause a bit of a slow down for reverts -- I believe drush batches them so does a few at a time.

das-peter’s picture

So, now on every feature rebuild/revert, alll the entity defaults are rebuilt?

Indeed, we added code that invokes a general rebuild - which also covers defaults not handled by features. If there are many default entities outside of features you will notice a slow down I guess.

hefox’s picture

If you got 20 features, and only one has entity exportables, it'll do a rebuild each time any of those features are rebuilt, correct?

For example, during batched module enable during an install of a distro

das-peter’s picture

All I can confirm is that it will do a full defaults rebuild whenever hook_features_post_restore() is invoked. "Full" means all exportable entity types wherever they are provided (features or not).

hefox’s picture

That means whenever _features_restore with op == rebuild

That will slow down install of distros in that situation (not sure which distros that is/ what components are provided by entities). Probably would be better to test if the features being reverted provides an exportable provided by entities

a.milkovsky’s picture

Previously "Full" rebuild was executed on every cache clear. With this patch performance of cache clear is really increased. But @hefox is right, feature revert performance is definitely decreased.

fago’s picture

Maybe, is there a way we can improve that during batches and skip it if we did it already during a batch? Sounds like we should create a follow-up issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

osopolar’s picture

Wondering, why is entity_rebuild_on_flush not set to FALSE by default? Are there any negative side-effects setting entity_rebuild_on_flush to FALSE?

osopolar’s picture

Status: Closed (fixed) » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

kenorb’s picture

kenorb’s picture