Improve performance when menu link value matches with the original value

Shouldn't be worked on until after #2301319: MenuLinkNG part5: Remove dead code; and party!

Yes we should do this! It saves over 12000 queries on a standard install! See https://blackfire.io/profiles/compare/e15b63b7-51bb-48a5-b3fe-b0ab514c78...

Given that I think this should be a critical task - if it is green and things look good.

Small mistake... but the improvements to Standard Install are just as real - https://blackfire.io/profiles/compare/4bf0ed62-e87d-4195-8148-c0f4c66e3c...

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -282,27 +282,29 @@ public function save(array $link) {
+    $original = $this->loadFull($link['id'], FALSE);

@@ -720,12 +722,14 @@ public function load($id) {
-  protected function loadFull($id) {
-    $loaded = $this->loadFullMultiple([$id]);
+  protected function loadFull($id, $unserialize = TRUE) {
+    $loaded = $this->loadFullMultiple([$id], $unserialize);

@@ -734,19 +738,23 @@ protected function loadFull($id) {
-  protected function loadFullMultiple(array $ids) {
+  protected function loadFullMultiple(array $ids, $unserialize = TRUE) {

Instead of making these internal API changes we could inline the query into doSave(). That would mean no protected API change.

Implementing #10 - same improvements https://blackfire.io/profiles/93a2b992-7257-4e0d-a81f-4e546187da1e/graph

Tested returning an empty array instead of $affected_menus. It made no performance difference to drush site-install standard. Given that and the fact that it is out-of-scope for this issue anyway - removing the @todo.

Discussed with @cilefen we agreed that the size of the performance improvements make this a critical task.

Fixing comments. Thanks @cilefen.

That is a really nice optimization!

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -282,27 +282,39 @@ public function save(array $link) {
+    // self::preSave() and, therefore, we need to avoid unserialization of
+    // certain fields.

Can we explain what these certain fields are, / why these specific fields?

Updated to try to explain what is going on and why.

I'm not sure if it's worth accounting (or micro-optimizing) for, but two arrays with the same data but keys in different order would serialize to different strings and not be considered equal here.

Avoiding that would mean sorting the fields (at least if they are arrays) before serializing?

re #18 @pwolanin well as the profiling shows that wouldn't have much impact on the installer so I'm not sure the cost of sorting before serialising in ::preSave() is worth it.

I'm not sure if it's worth accounting (or micro-optimizing) for, but two arrays with the same data but keys in different order would serialize to different strings and not be considered equal here.

I'm not sure what should cause a different sort order to be honest.

I would RTBC it, but I'll give @pwolanin the honour.

Just re-reviewing. Can be certain that the number of array keys is identical? Otherwise the test using array_diff_assoc() may not be correct:

php > $a1 = ["a" => "green", "b" => "brown"];
php > $a2 = ["a" => "green", "b" => "brown", "c" => "yellow"];
php > print_r(array_diff_assoc($a1, $a2));
Array
(
)

Note on line 367 of MenuTreeStorage we are not assured that all the fields are set in $link, only that extra fields are removed.
$fields = array_intersect_key($link, $schema_fields);

strict comparison the return value of array_diff_assoc() with an empty array is slight overkill?

@pwolanin nice catch. New patch that addresses that. See https://blackfire.io/profiles/compare/da938773-9e95-48fc-b6d2-214d7a1222... for a comparison of HEAD vs the patch for a standard install.

The reason I didn't remove mlid from $original is because $original is used later on.

Discussed this issue with both @dawehner and @catch. We didn't manage to come up with better ideas so I've improved the docs to say why the array_diff_assoc() approach is valid.

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -282,27 +282,44 @@ public function save(array $link) {
+      if (array_diff_assoc($fields, $original) == [] && array_diff_assoc($original, $fields) == ['mlid' => $link['mlid']]) {
+        return $affected_menus;
+      }

This is returning a non-empty array even if no data was changed.

This means possibly unneeded cache purging in the calling function. Maybe better to return an empty array and document that that's possible? I thought an earlier patch was returning an empty array?

@pwolanin maybe but that would be a behaviour change that I think is out-of-scope. It is still the affected menu after all. I think that returning an empty array here should maybe be considered in a followup issue. In practice returning an empty array here had 0 impact on menu rebuilds because \Drupal\Core\Menu\MenuTreeStorage::saveRecursive() ignores this completely.

So, when this issue was first being discussed, the text I read about it was:

https://blackfire.io/profiles/compare/e15b63b7-51bb-48a5-b3fe-b0ab514c78... 12,000 less queries on a standard install

However, this is ambiguous. I read it as "12,000 fewer queries [in some circumstance] for the Standard profile". However, it appears from inspecting https://blackfire.io/profiles/compare/4bf0ed62-e87d-4195-8148-c0f4c66e3c... more closely that it means during installation. I don't really think improvements that only affect the speed of the installer can be considered critical. While speeding up the installer will improve test run time significantly (thus saving resources and occasionally developer time when waiting on CI test results), slow installer performance is not as disruptive as a random test failure, which is the only reason I could come up with for having an installer performance improvement be impactful.

So, downgrading to major unless there's also a significant runtime improvement, in which case it should be major or critical according to where it fits on https://www.drupal.org/core/issue-priority#critical-task.

@xjm
Does that mean we should clarify that bit in the document? Mentioning that this is just about user facing pages (not admin pages/installer/test UI etc.)

For:

Does that mean we should clarify that bit in the document? Mentioning that this is just about user facing pages (not admin pages/installer/test UI etc.)

I don't think it's not about admin pages. Bad performance on admin pages makes a site unusable. I think it's just a question of runtime/production configurations vs. something that's developer-facing only. However, we can add that. I'll check with @catch.

We agreed to repromote this issue for now and triage it more since it could indeed have a valuable runtime impact (needs profiling) as it affects runtime menu rebuilds which happen in lots of admin contexts and have been weak points in the past to take down big sites.

We tried this draft update to the policy:
https://www.drupal.org/node/45111/revisions/view/10314345/10471220

What does this needs profiling for?

I've done a cache rebuild via the UI profile - https://blackfire.io/profiles/compare/23694618-2da7-44ed-9351-de0a71aeec... -
this is the saving that we'll see on standard for anything that causes a menu rebuild. And the savings will be bigger on real sites.

Adding more profiling of what happens when \Drupal\Core\Menu\MenuTreeStorage::rebuild() is called.

Here's a profile of calling \Drupal::service('plugin.manager.menu.link')->rebuild(); https://blackfire.io/profiles/compare/564ce396-a3e0-47c0-90d4-3d25f42bbd... and using blackfire's sampling to improve accuracy. The script looks like this:

$samples = 10;
$blackfire = new \Blackfire\Client();
$config = new \Blackfire\Profile\Configuration();
// set the profile title
$config->setTitle('MenuTreeStorage::rebuild()');
$probe = $blackfire->createProbe($config, false);

for ($i = 1; $i <= $samples; $i++) {
  // enable instrumentation
  $probe->enable();
  \Drupal::service('plugin.manager.menu.link')->rebuild();
  // finish the profile so that another one will be taken on the next loop iteration
  $probe->close();
}
// send the results back to Blackfire
$profile = $blackfire->endProbe($probe);

Short version of #32 is it saves 284 queries with the standard profile on menu rebuild, which has a lot less menu links than real sites with lots of modules installed.

Even if those queries were half a millisecond each, that'd be 142 milliseconds saving, but also they'll be write queries which is more of an impact anyway.

Looked over the patch again. Think this is good to move forward.

Maybe we could add a little testing in any follow-up to change the return since returning the empty array when nothing changed would be something verifiable in terms of behavior.

Maybe we could add a little testing in any follow-up to change the return since returning the empty array when nothing changed would be something verifiable in terms of behavior.

I think this is a good idea, but let's do it in this issue rather than a followup. At least, I'd like to know if it's feasible here. Tests are a core gate.

I was about to argue that we should not change the return value, in the context of https://symfony.com/doc/current/contributing/code/bc.html

But then I realized that we change the behaviour of the function here, the patch as of now no longer fullfills its documentation:

   * @return array
   *   The menu names affected by the save operation. This will be one menu
   *   name if the link is saved to the sane menu, or two if it is saved to a
   *   new menu.

Given that changing the return value, or adapting the documentation is helpful. I went with both to clarify what happens.

Cleaning up the docs a bit. Tests looks good and covers what was asked for in #36.

Personally I think this change in #37 is wrong. Because, before when the value was the same the return value still contained the affected menu. We're now changing the external behaviour for an internal behaviour change. The internal behaviour of \Drupal\Core\Menu\MenuTreeStorage::doSave() shouldn't be tested either. What's important is the menu saving is tested and it is. This patch does not change whether or not a menu is saved (ie exists in the database) it changes whether or not we do unnecessary database updates. This is totally an internal implementation detail.

The patch in #23 is very unlikely to break anything. The patch in #37 might because something somewhere might be relying on the cache invalidation that occurs when a menu link is saved even though nothing on the menu link itself has changed. Therefore I'd really like the changes in #37 and this comment to be a follow-up. Because #23 can go into 8.3.x but I don't think this change should.

Discussed with @dawehner. We agreed that changing the return value in this issue is an out-of-scope change. It is separate issue from the internal detail of whether or not we do an additional database query to update values to exactly the same values they already were. I've opened #2876343: Return an empty array from \Drupal\Core\Menu\MenuTreeStorageInterface::save() if no storage update done to discuss this.

We could add a very tricksy unit test to test the internal implementation details of \Drupal\Core\Menu\MenuTreeStorage::doSave() but I think that will lead to harder to change code and shouldn't be done. We'd have to mock the db layer. A better test for this type of thing would be a have generic performance tests that count the number of queries in say a standard install and alert us when we have something silly happen.

Re-uploading #23.

A better test for this type of thing would be a have generic performance tests that count the number of queries in say a standard install and alert us when we have something silly happen.

#638078: Automated performance testing for core

I don't think it's going to matter in practice whether we change the return value here, however I can see the arguments for isolating the change to a separate issue if we do. Field API has optimisations to avoid updating identical values in tables, but the API itself behaves as if it has.

I don't think it's going to matter in practice whether we change the return value here, however I can see the arguments for isolating the change to a separate issue if we do. Field API has optimisations to avoid updating identical values in tables, but the API itself behaves as if it has.

Always +1 for smaller steps. It always reduces the risks.

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -282,27 +282,44 @@ public function save(array $link) {
+    // Get the existing definition if it exists. This does not use
+    // self::loadFull() to avoid the unserialization of fields with 'serialize'
+    // equal to TRUE as defined in self::schemaDefinition(). The makes $original
+    // easier to compare with the return value of self::preSave().
+    $query = $this->connection->select($this->table, $this->options);
+    $query->fields($this->table);
+    $query->condition('id', $link['id']);
+    $original = $this->safeExecuteSelect($query)->fetchAssoc();
+

I was thinking about that for a while. Should we introduce a new method which does that, and share the code? I don't think this is the right thing to do. Even a protected method is some sort of abstraction we need to support in the future. The code here is not complex, while still really explicit. We need to be less scared of copying code.

Committed 37bbe3f and pushed to 8.4.x. Thanks!

This should be harmless for 8.3.x, but there's no bug, so moving to PTBP since I can't decide at the moment.

Decided. Cherry-picked to 8.3.x, too.