Only set router rebuild needed when something related to routing actually changes

This has to be discussed a bit

* Should cache plugins be able to hook into this process? They might store the caches somewhere.
* The block display has to clear the block caches, too.

+++ plugins/views_plugin_display_page.incundefined
@@ -503,4 +503,16 @@ class views_plugin_display_page extends views_plugin_display {
+    parent::save();

Should probably be parent::delete()?

+++ plugins/views_plugin_display_page.incundefined
@@ -503,4 +503,16 @@ class views_plugin_display_page extends views_plugin_display {
+    menu_rebuild(); // force a menu rebuild when a view is deleted.

Rather than calling a menu rebuild, it's preferred to use variable_set('menu_rebuild_needed', TRUE); which will automatically rebuild the menu on the next request.

+++ plugins/views_plugin_display_page.incundefined
@@ -503,4 +503,16 @@ class views_plugin_display_page extends views_plugin_display {
+  function delete() {

OMG you suck!

Just chiming in to note the severity of the issue. Every time we save a view, our website comes to a grinding halt. Watching the DB live indicated that it was hits to menu/cache related tables.

We are seeing the same thing in D7 ... views take forever to save, web site grinds to a halt. Looking at the semaphore table reveals that the menu_rebuild keeps firing .... sigh.

@incursio
The real solution is not to edit views on the actual page, but using exported views or some other proper deployment functionality.

This *is* on a development machine.

incursio, I temporarily resolve my problems by commenting out the menu rebuild/all cache flushing on view save and instead trigger those manually. I believe views may be triggering menu_rebuild multiple times: #1885668: Slow saving of views and site lockups

Here is a start at something for 7.x-3.x.

I have added a parameter to views_invalidate_cache() to allow bypassing of rebuilding the menu:

/**
 * Invalidate the views cache, forcing a rebuild on the next grab of table data.
 *
 * @param bool $rebuild_menu
 *   Indicate whether the menu should be rebuilt. This should only be done when
 *   we expect the menu to have changed.
 */
function views_invalidate_cache($rebuild_menu = TRUE) { ... }

I have defaulted it to TRUE so that other modules calling it continue to get the same behaviour.

I've then updated view::save() and view::delete() to no rebuild the menu and pass on to the displays to be do any additional cache invalidation with a new method in views_plugin_display:

  /**
   * Invalidate any appropriate caches for this display.
   */
  function invalidate_cache() { }

Displays can override this to do things such as marking the menu for rebuild in views_plugin_display_page

There are a whole load of ways this could be better, but hopefully it's a start:

• views_plugin_display_page only rebuild the menu when the path has changed
• Other calls to views_invalidate_cache() can only rebuild the menu when necessary
• Page/block cache could also be included

Lets try that again...

Patch from over in #2226541: views_invalidate_cache() clears too much - closed as a dupe of this.

Just in general, try to get inspired by the previous patches as well.

1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
   @@ -420,6 +420,8 @@ public function submitOptionsForm(&$form, &$form_state) {
    
        if ($form_state['section'] == 'path') {
          $this->setOption('path', $form_state['values']['path']);
   +      // Set the menu as needed to be rebuilt.
   +      \Drupal::service('router.builder')->setRebuildNeeded();
        }
      }
   

   I don't think we should trigger a menu rebuild in the UI. Instead we should try to figure that out while saving / updating / importing the view.

2. +++ b/core/modules/views/lib/Drupal/views/ViewsData.php
   @@ -194,6 +195,22 @@ protected function prepareCid($cid) {
      /**
   +   * Gets an array of cache tags.
   +   *
   +   * @return array
   +   *   An array of cache tags.
   +   */
   +  protected function getCacheTags($key = NULL) {
   

   let's describe what the $key is doing.

3. +++ b/core/modules/views/views.module
   @@ -525,46 +525,23 @@ function views_language_list($field = 'name', $flags = Language::STATE_ALL) {
   + *
   + * @deprecated @todo
   

   Let's ship with this.

Fair, that was just a rough patch though, as you can probably tell.

Is this still a problem ever since the menu homestretch patch landed?

views_invalidate_cache() still rebuilds the router regardless of whether the current View even has a route (i.e. a page display). It also clears block cache regardless of whether the block display plugin is involved. If we think those are separate issues, we could reopen #2226541: views_invalidate_cache() clears too much

Berdir has confirmed that this is quite a big performance problem in #2241377-28: [meta] Profile/rationalise cache tags.

#8: This is I think strongly related to #2425259: Router rebuild lock_wait() condition can result in rebuild later in the request (race condition), which can also trigger rebuild again and again.

All thats needed is something doing a menu_get_item() while the cache is being rebuild ...

I don't think this issue is about that problem.

This is about the following:

a) Assuming you have a view that only has a block, then saving that view will still trigger a router rebuild.
b) Assuming you have a view with a page display, but you are not changing any option that would require a menu rebuild, it will still do that.

(menu is somewhat misleading here and 7.x terminology)

Fabianx's issue is more related to this, as is #356399: Optimize the route rebuilding process to rebuild on write I think.

Also, there are two distinct problems with that I reported I think.
a) Multiple router rebuilds, which I've noticed in some cases, e.g. when saving views, but I don't think it's the fault of views.
b) menu cache tag invalidations, which did not happen during router rebuilds but at other times.. possibly saving menu links.

Just some ideas.

Another try.

Meh.

Let's try.

This could fix a couple of the failures.

"Invalidate the views cache, forcing a rebuild on the next grab of table data."

I'm not clear what we mean by 'table data'. I think we mean the views config entities themselves.

There we go.

Nice, this looks very promising!

Looking forward to views savings not taking forever when you just want to make a tiny change somewhere in a field handler.

1. +++ b/core/modules/views/src/Views.php
   @@ -534,4 +535,25 @@ protected static function t($string, array $args = array(), array $options = arr
   +    // Reset the RouteSubscriber from views.
   +    \Drupal::getContainer()->get('views.route_subscriber')->reset();
   

   Why use getContainer() here?

2. +++ b/core/modules/views/src/Views.php
   @@ -534,4 +535,25 @@ protected static function t($string, array $args = array(), array $options = arr
   +    Cache::invalidateTags(['views_data']);
   

   can we call resetCache() on the service here instead? That can also reset static caches.

Looking forward to views savings not taking forever when you just want to make a tiny change somewhere in a field handler.

+1 :)

The long time it takes to save a view (in D7 & D8) has always puzzled me. Sounds like this will make that go away :)

Thank you @berdir for the review.

Some work on it.

+++ b/core/modules/views/src/Entity/View.php
@@ -220,7 +220,13 @@ protected function generateDisplayId($plugin_id) {
+    $empty_array = [];

Do you mean array()?

@Sharique.
Well, I mean []. In PHP 5.4 you can use [] as alternative to array(), see http://php.net/manual/de/migration54.new-features.php

Thanks dawehner, I did not know about that.

Some work here.

Let's see whether we managed to find all the failures.

Let's see whether this is all we need.

Reviewed with paboden.

I triaged with @paboden at Drupalcon Los Angeles Sprints.

The issue summary is understandable and up to date.

To reproduce, I set a breakpoint on function setRebuildNeeded() in RouteBuilder.php and saved a view with no changes. setRebuildNeeded() was called during the save, indicating an unnecessary router rebuild.

The last patch only had warnings during the last simpletest run. We might only need a reroll.

In postSave(), there is a comment before views_invalidate_cache() is called:
// @todo Remove if views implements a view_builder controller.

But does the router rebuild still cause crippling performance? If not, maybe this is no longer a major bug.

Rerolled.

#57: Generally we should try to avoid unnecessary work.

Reroll. Didn't look to see b/c of which commits.

$ git checkout 29429bd
$ git apply --index views_rebuilds_the_menu-941970-58.patch
$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: views_rebuilds_the_menu-941970-58.patch
Using index info to reconstruct a base tree...
M	core/modules/rest/src/Plugin/views/display/RestExport.php
M	core/modules/rest/src/Plugin/views/style/Serializer.php
M	core/modules/views/src/Entity/View.php
M	core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
M	core/modules/views/src/Plugin/views/display/DisplayPluginInterface.php
M	core/modules/views/src/Plugin/views/display/Page.php
M	core/modules/views/src/Plugin/views/display/PathPluginBase.php
M	core/modules/views/src/Tests/Plugin/AccessTest.php
M	core/modules/views/src/Tests/Plugin/DisplayTest.php
M	core/modules/views/src/ViewExecutable.php
M	core/modules/views/src/Views.php
M	core/modules/views/tests/modules/views_test_config/test_views/views.view.test_disabled_display.yml
M	core/modules/views/views.module
Falling back to patching base and 3-way merge...
Auto-merging core/modules/views/views.module
Auto-merging core/modules/views/tests/modules/views_test_config/test_views/views.view.test_disabled_display.yml
Auto-merging core/modules/views/src/Views.php
Auto-merging core/modules/views/src/ViewExecutable.php
Auto-merging core/modules/views/src/Tests/Plugin/DisplayTest.php
Auto-merging core/modules/views/src/Tests/Plugin/AccessTest.php
Auto-merging core/modules/views/src/Plugin/views/display/PathPluginBase.php
CONFLICT (content): Merge conflict in core/modules/views/src/Plugin/views/display/PathPluginBase.php
Auto-merging core/modules/views/src/Plugin/views/display/Page.php
Auto-merging core/modules/views/src/Plugin/views/display/DisplayPluginInterface.php
Auto-merging core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
Auto-merging core/modules/views/src/Entity/View.php
Auto-merging core/modules/rest/src/Plugin/views/style/Serializer.php
CONFLICT (content): Merge conflict in core/modules/rest/src/Plugin/views/style/Serializer.php
Auto-merging core/modules/rest/src/Plugin/views/display/RestExport.php
CONFLICT (content): Merge conflict in core/modules/rest/src/Plugin/views/display/RestExport.php
Failed to merge in the changes.
Patch failed at 0001 views_rebuilds_the_menu-941970-58.patch
The copy of the patch that failed is found in:
   /Users/matt/Documents/PhpStorm/drupal-8.0.x/.git/rebase-apply/patch

Simple conflicts in the use sections and where new code went.

I can't get Drupal\config\Tests\ConfigImportAllTest to complete locally to see what is really going on.

Reroll, didn't apply anymore. Didn't change anything else except remove some double newlines.

Just a guess:

+++ b/core/modules/views/src/Entity/View.php
@@ -346,9 +352,25 @@ protected function addCacheMetadata() {
+    $executable = $this->getExecutable();
...
+    }
+    foreach ($executable->displayHandlers as $display_id => $display_handler) {

Do we need to call $executable->initDisplay(); before the foreach?

@joelpittet sounds like a good guess, lets see what it does. Like @mpdonadio I can't get Drupal\config\Tests\ConfigImportAllTest to pass cleanly locally, but fails with timeouts and stuff so seems unrelated to this.

Added missing use and cleaned up a white space error that snuck in.

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -345,6 +346,18 @@ public function render() {
   +    if (method_exists($style_plugin, 'postSaveView')) {
   +      $style_plugin->postSaveView($original_display);
   +    }
   

   Given that StylePluginBase has no interface it would be at least save to add it to there, so we don't need a methodExists() check. Ideally though we would have something like ViewChangesInterface which would contain all that kind of methods.

2. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -139,6 +141,27 @@ public function render() {
      /**
   +   * Act on saving a view.
   +   *
   

   ... it would be really helpful to explain what we actually check in that 12 lines of code.

@dawehner something like this?

1. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -141,20 +141,16 @@ public function render() {
   +    // We need a router rebuild if the available formats have changed.
   

   ..., because the formats are stored on the route.

2. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
   @@ -826,6 +827,14 @@ public function query() {
   +   *   (optional) The original display.
   

   Do you mind documenting that the original display is empty when we just created a new view?

Updated per #73

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -345,6 +347,17 @@ public function render() {
+   */
+  public function postSaveView(DisplayPluginInterface $original_display = NULL) {
+    parent::postSaveView($original_display);
+
+    /** @var StylePluginBase $style_plugin */
+    $style_plugin = $this->getPlugin('style');
+    $style_plugin->postSaveView($original_display);
+  }

This sounds something which should live in DisplayPluginBase

@dawehner yeah that makes sense. Rerolled for #76 and since #2464657: Remove unnecessary cache clear in Views tests landed, that bit can be taken out too.

So we are adding this to DisplayPluginBase but we don't add it to StylePluginBase, but still call the method?

+++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
@@ -838,6 +839,15 @@ public function query() {
+   */
+  public function postSaveView(DisplayPluginInterface $original_display = NULL) { }
+

@dawehner We don't? Or are you referring to something else? If so, could you elaborate a bit? Patch didn't apply to 8.1.x and 8.2.x so rerolled for that, #77 still applies to 8.0.x.

@Lendude No idea what I was smoking on sunday :)

+++ b/core/modules/views/src/Entity/View.php
@@ -233,7 +233,13 @@ protected function generateDisplayId($plugin_id) {
+    $empty_array = [];
+    if (isset($this->display[$display_id])) {
+      return $this->display[$display_id];
+    }
+    else {
+      return $empty_array;
+    }

I'm wondering whether we really want to do this here ... isn't it a sign of some brokenness if you call out to a display that doesn't exist? This implementation here also doesn't care about the reference properly. You would rather setup $this->display[$display_id] instead.

@dawehner haha! No worries. At first glance I'd say I agree with you that that bit looks bad/weird, I'll take a look at it when I get some time.

@dawehner so do you mean something like this?

+++ b/core/modules/views/src/Entity/View.php
@@ -233,13 +233,12 @@ protected function generateDisplayId($plugin_id) {
+      // Setup the requested display. Since we have no information other then
+      // the ID, set it up with defaults.
+      $this->addDisplay('page', NULL, $display_id);

We add a page display by default? This feels weird

This is the default in ViewEntityInterface

public function addDisplay($plugin_id = 'page', $title = NULL, $id = NULL);

and since we want to set $id, I figured using the default for $plugin_id for that method made sense. Unless there is a 'Broken' display, but don't know if there is anything like that.

Fair point. In general i'm still wondering why this issue requires us to adapt getDisplay in the first place.

Yeah I was wondering the same thing, it seems unrelated. It first showed up in your patch in #42 so I assumed it was causing some tests to fail.

@Lendude
Yeah that sounds like a workaround for some missing display.

Ok so lets see what still fails, and why, when we take it out.

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2679,6 +2681,16 @@ protected function isBaseTableTranslatable() {
+   */
+  public function postSaveView(DisplayPluginInterface $original_display = NULL) {
+    /** @var StylePluginBase $style_plugin */
+    $style_plugin = $this->getPlugin('style');
+    $style_plugin->postSaveView($original_display);
+  }
+

I think we should also detect whether we got new arguments ... as this will change the required route as well

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -536,4 +536,29 @@ public function remove() {
+      $determining_options = ['path', 'route_name', 'arguments', 'access', 'enabled'];
...
+    }

@dawehner Isn't that covered in PathPluginBase::postSaveView()? Is there ever a need to rebuild routes when not using/extending PathPluginBase?

Yeah not sure, I was probably just blind.

+++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
@@ -837,6 +838,15 @@ public function query() {
+  /**
+   * Act on saving a view.
+   *
+   * @param \Drupal\views\Plugin\views\display\DisplayPluginInterface $original_display
+   *   (optional) The original display. When saving a new view this will be
+   *   empty.
+   */
+  public function postSaveView(DisplayPluginInterface $original_display = NULL) { }
+

We've entered the rc phase for 8.1.x and this is an API addition therefore moving to 8.2.x.

1. +++ b/core/modules/views/src/Plugin/views/display/Block.php
   @@ -371,4 +371,16 @@ public function remove() {
   +    if (\Drupal::moduleHandler()->moduleExists('block')) {
   +      \Drupal::service('plugin.manager.block')->clearCachedDefinitions();
   

   The module handler and block manager can be injected instead now?

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2679,6 +2681,16 @@ protected function isBaseTableTranslatable() {
   +    $style_plugin->postSaveView($original_display);
   

   This could be quite important that it happens, so I think maybe the style plugins should be handled in View::postSave() too. Rather than relying on the parent method being called.

3. +++ b/core/modules/views/src/Tests/Plugin/AccessTest.php
   @@ -87,20 +88,24 @@ function testStaticAccessPlugin() {
   +    $view->destroy();
   

   Thank you! nice.

4. +++ b/core/modules/views/src/Views.php
   @@ -535,4 +535,25 @@ protected static function t($string, array $args = array(), array $options = arr
   +    $module_handler = \Drupal::moduleHandler();
   

   Doesn't seem any point in setting a variable for the module handler here. Can just be a one liner.

5. +++ b/core/modules/views/views.module
   @@ -459,21 +459,7 @@ function views_field_config_delete(FieldConfigInterface $field) {
    function views_invalidate_cache() {
   

   Maybe we should be deprecating this now too, with a note that code should just use Views::invalidateCache()

Thanks for the feedback @damiankloip! So, something like this then?

+++ b/core/modules/views/src/Entity/View.php
@@ -360,6 +361,9 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
       $display_handler->postSaveView($original_display_handler);
+      /** @var StylePluginBase $style_plugin */
+      $style_plugin = $display_handler->getPlugin('style');
+      $style_plugin->postSaveView($original_display_handler);
     }
 

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2680,11 +2679,7 @@ protected function isBaseTableTranslatable() {
-  public function postSaveView(DisplayPluginInterface $original_display = NULL) {
-    /** @var StylePluginBase $style_plugin */
-    $style_plugin = $this->getPlugin('style');
-    $style_plugin->postSaveView($original_display);
-  }
+  public function postSaveView(DisplayPluginInterface $original_display = NULL) { }
 

So what about supporting all plugin types?

@dawehner so something like this? Moved postSaveView to Drupal\views\Plugin\views\PluginBase and then call it for all plugins that are present in each display.

Thanks @paboden and @mortona2k triaging this issue last year, and thanks @Lendude for helping the maintainers with recent work on the issue.

@tim.plunkett, @dawehner, @alexpott, and I discussed this issue at DrupalCon New Orleans and decided to defer deciding whether it should remain major for two reasons:

1. Whether or not this is a major bug depends of the performance impact of the change. So, marking for profiling.
2. Rebuilding too much is better than rebuilding too little, so unless the performance cost of the extra rebuilds is significant, we are in an okay situation currently. It's still worth making the improvement, but we would need to test carefully to ensure we were not rebuilding too little after this patch.

Given we need profiling, let's move it to needs work.

Menu rebuilds used to take several seconds but we've done a lot to optimise that since 2010:

1. Router rebuild and link discovery are separated, so that link discovery doesn't block other request routing (and sometimes doesn't happen at all).

2. setRebuildNeeded() means that for configuration deploys instead of every views save triggering a rebuild, there should only be one.

I think that means it's time to downgrade this from bug to task. However from what I can see the approach would still help a couple of cases:

1. Speed up saving of views for admins/site builders (when they're likely to be the person triggering the next request that rebuilds the menu).
2. Optimise away that last unnecessary rebuild on config deploys even if it's just 1 instead of views saves * 1.

As we deprecate the procedural code, I'll, make this part of the #3566536: [meta] eliminate core .module files effort. Will try to revive it

Hide patches

Fixed IS

Ready for review

Unassigning

Went through this a few times, I think this makes a lot of sense now. It's separating the cache invalidation and the route rebuild so each display type can handle the part they need to.

Since this isn't a straight replacement I think we should do a CR here and reference that one.

I created a first pass at the CR and suggestions to swap it. If the CR looks good we can apply those and update https://www.drupal.org/node/3566774 to remove the reference to this function.

Once that is done I think this is ready!

Small question on the MR.

Just answered!

Thank you for cleaning up the comment and block plugin change, I think this is ready!

Added some comments to the MR.

Ready for a new review

All comments have been addressed, looks good to me!

Added some more thoughts to the MR. Not entirely sure about the answers - needs discussion.

The de duplication seems like a good idea.

I'll add my few cents here.. This branch creates new API for basically all views plugins in the form of postSaveView(). Since it'll be always there, possibly contrib will use it now so we have no way of predicting what will happen there. Deduping is a bit dangerous as it may happen some callbacks will actually need to be called multiple times, just with different arguments or even same arguments, we have no idea how will people use the new API. I'd pass some initially empty $context array by reference in each call to postSaveView() and handle possible duplication on the plugin level.

BTW.. Seems also View::routeRebuildNeeded() can be called multiple times, even if harmless, it's not needed so if the $context solution will be the preferred one, calling View::routeRebuildNeeded() only once should be included on the plugin level too. I'd move that method to core/modules/views/src/Plugin/views/PluginBase.php probably (protected).

About the base plugin class method postSaveView() concern:

Problem:
Views has two plugin families on one base class: HandlerBase extends PluginBase implements ViewsPluginInterface, so handlers (field, filter, sort, …) are plugins — but getPlugin()/getAllPlugins() only cover 'plugin' types, while handlers are reached via the separate getHandlers()/getAllHandlers(). So postSaveView(), added to ViewsPluginInterface+PluginBase, is inherited by handlers that View::postSave() never iterates (it only walks getPlugin()), and Block::postSaveView() clears the block cache once per block display (N times).

Proposed solution:
Move postSaveView() to a dedicated opt-in PostSaveViewInterface (with a by-ref $context); remove it from ViewsPluginInterface and PluginBase.
Implementers (PathPluginBase, Block, REST Serializer) still perform their own effect, but record it in $context so a peer that would repeat a view-global effect skips it.
View::postSave() iterates display handler + getAllPlugins(TRUE) + getAllHandlers(TRUE) (same coverage as calculateDependencies()), dispatches to instanceof PostSaveViewInterface, and stays generic — it seeds $context but never acts on it, as in my last comment - plugins use it to avoid duplication.

Why:
Performance: block cache cleared / routes rebuilt once per save, no static state.
Controlled API: opt-in interface instead of forcing the method onto all ~19 types via PluginBase.
Complete coverage: handlers can now participate, and $only_overrides = TRUE visits each configured plugin/handler once, not once per inheriting display.

Since there's no more discussion here, setting this to NW.