MenuLinkNG part4: Conversion

Asked for in #2301239-39: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage 3.
add specific test coverage for encodeId() in MenuLinkOverrides

#39 3, those were just added to part 1. great.

Here's the full remaining patch through step 4, the patch for step 4 on top of step 3, and the incremental change since Alex's 1st patch on the issue with some fixes and method name changes from earlier parts.

In order for this part 4 patch to be committed (which is really the one that brings it all together), #1805054-125: Cache localized, access filtered, URL resolved, and rendered menu trees needs to be answered:

In #73, patch+hack was same as HEAD, now it's 10% slower. Probably due to the merging of Wim's work (better code sometimes has a perf cost). Would be good to know what specifically is responsible for that to make an informed decision on whether to accept it or try to optimize.

That's what this comment wants to do. Since it's easy to make mistakes in profiling, I included sufficient information for anyone to reproduce this.

The very short answer is: the patch now removes menu block which it didn't do previously, plus menu tree loading/building/rendering became ~10% slower (most likely due to the added abstractions, which is necessary for #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees).

… and in doing this profiling, I've uncovered a wider performance problem.

Tested with HEAD === 50ac4700e21f401fea8cca2379e111a912173a95.

For those of you who don't want to read the full analysis, I've put the summary/conclusion and takeaways at the top — but please do read everything if you're going to comment regarding the profiling, otherwise you're not acting on the full info.

Summary/conclusion

1. From step 1: I repeated effulgentsia's test and found the overall regression to be 29%, not 25%. Most likely explained by the different environment (hardware, PHP version …) and the used metric (ab).
2. From step 2: I benchmarked & profiled a response that doesn't render any menu, and in fact doesn't render any HTML, and I still found a 10% regression (note that this was already present before my patch was merged). The root cause is: more services are instantiated, even though they're not used! This is a wider problem in Drupal core, and will only get worse when you add contrib modules.
   A possible solution is lazy services — see #1973618-27: DIC: Lazy instantiation of service dependencies (ProxyManager for "proxy services").
3. From step 3, optimization 1: this is the most apple-to-apple-like comparison possible. Instead of the 10% regression that effulgentsia found, I found a ~20% regression (though indeed a ~10% regression in number of function calls).
4. From step 3, optimization 2: in HEAD, SystemMenuBlock is cached, in pwolanin's original patch this is also the case, but now that my patch is merged, this is no longer the case. HEAD is incorrectly caching menu blocks per role, hence e.g. entity access checks are not running at all, which yields incorrect results. So for this optimization, I enabled the same caching again.
   • Benchmark: 29% -> 8% regression.
   • Profiling: function calls: 2.9% regression, wall time: 8.6% regression.
5. From the intermezzo: With the two simple optimizations from above applied, which brought us quite close already to where we need to get (no regression) considering where we came from, I roughly simulated part of the performance improvement #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees will bring (I disabled the rendering of primary and secondary menus, since they will become cached as well). This singlehandedly turns the 8% regression into a 3% improvement — for the super simple primary and secondary menus that core ships with — in the real world, the difference will be much bigger.
6. From step 3, optimization 3: in HEAD, the typical menu tree parameters (which depends on the active trail and the links marked as "expanded") are cached per-page. When cleaning that up, pwolanin and I decided to not prematurely optimize that, so in the current patch there's no caching for it anymore. Restoring caching for that (paramscache-do-not-test.patch) is something we probably want to include as part of this path. Doing that
   • Benchmark: 29% -> 8% -> 5% regression.
   • Profiling: function calls: no regression (9 less!), wall time: 3.6% regression.
7. From the observation: menu tree loading/transforming/rendering has become slightly slower (~10%) — probably due to the added abstractions needed for #1805054. However, there's a known inefficiency in HEAD where the main and secondary menus are rendered twice, which of course means that this slowdown is stressed excessively. #1805054 will fix that, but if we simulate that by statically caching the rendering of these menus (navlinks_stupidity-do-not-test.patch), we can get a better sense of what #1805054 will bring us. We also get a better sense how hard or simple it will be to make improvements that offset the regression caused by this patch.
   • Benchmark: 29% -> 8% -> 5% -> 2.8% regression.
   • Profiling: function calls: 1.2% improvement, wall time: 0.2% improvement.

Takeaways

Rendering menus is excessively slow

It's clear that Drupal 8 spends an inordinate amount of time on rendering menus — both before and after this patch. Roughly 20% of the total page generation time is spent on rendering menus, both before and after this patch.

Menu rendering becomes 10% slower

To allow it to be made an order of magnitude faster by making menu rendering cacheable in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

Overall page generation time regression is surmountable

1. We MUST implement something like #2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache (step 3, optimization 1), but doing that is out of scope for this issue. That will also help all other rendered links.
2. We COULD bring back menu block caching to this patch (step 3, optimization 2), even though the results will continue to be wrong, or we could let #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees remove it.
3. We MUST include caching of the typical menu tree parameters in this patch (step 3, optimization 3).
4. We COULD include static caching of rendered primary/secondary menus; there's no harm in doing so, it soothes the pain this patch brings, and it would be removed by #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

With 2. and 3. as part of this patch and 1. added later, the effective regression is only 3.6% (this is what step 3, optimization 3 shows us).

The context you need to understand this profiling

This issue is one of 5 parts that #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module was split into. This issue represents part 4, which performs the actual conversion — the first three patches introduced the new code, without actually using it. Those patches landed after thorough code review.
For this patch to land, code review alone isn't sufficient; it also needs to prove that it introduces at most a minimal regression.

A minimal regression is acceptable because #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees promises to bring render caching of menus, which will bring significant performance improvements that would easily offset a minimal regression in this issue (see #1805054-97: Cache localized, access filtered, URL resolved, and rendered menu trees — 10 to 20% improvement of overall page load time, depending on the complexity of the page and menus).

However, to do that, I had to refactor MenuTree significantly. Unfortunately, pwolanin did the same in #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module. So we had to merge our work.

Before we merged our patches, effulgentsia did profiling at #2256521-73: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module and found this patch to be 15% slower. However, a large part of that can be attributed to the very inefficient LinkGenerator. With the hack in #2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache applied, this patch was deemed to be essentially equally fast as HEAD.

After I merged my patch though, effulgentsia repeated the above (so this patch + the hack of #2289319-2) at #2256521-125: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module and found a ~10% regression (without that hack, it's 25%). My goal was to identify the root cause of that ~10% regression.

Note that since effulgentsia did all benchmarking/profiling using the anonymous user, I did so as well.

Tooling :(

Let me start by saying that I think that determining if there is a performance regression by running ab is less than great. It's easy to get wildly varying results between different runs, even when doing 100 requests. I think it's more appropriate to compare the number of CPU instructions.
There's easily a ~10% difference between different ab runs!
Sadly, XHProf doesn't give us that information either. Because even in XHProf runs, I see quite large variance.

In short: the tooling is lacking.

Step 1: high-level benchmarking

1. Best of ten runs (ab -c1 -n100 http://domain/menu-bench, PHP 5.5.11, OpCache enabled, XDebug disabled, XHProf disabled):
   1. HEAD: 148.087 ms/request
      

                      min  mean[+/-sd] median   max
        Connect:        0    0   0.0      0       0
        Processing:   134  148  10.9    144     177
        Waiting:      123  136  10.1    133     164
        Total:        134  148  10.9    144     177
      
        Percentage of the requests served within a certain time (ms)
          50%    144
          66%    149
          75%    152
          80%    157
          90%    168
          95%    173
          98%    176
          99%    177
         100%    177 (longest request)
      

   2. patch (#2301317-6: MenuLinkNG part4: Conversion): 190.349 ms/request
      

                      min  mean[+/-sd] median   max
        Connect:        0    0   0.0      0       0
        Processing:   169  190  16.7    185     245
        Waiting:      158  178  15.9    173     229
        Total:        169  190  16.7    185     245
      
        Percentage of the requests served within a certain time (ms)
          50%    185
          66%    191
          75%    201
          80%    203
          90%    216
          95%    231
          98%    239
          99%    245
         100%    245 (longest request)
      

   This is a 29% regression.
   NOTE: one of the runs with the patch was 182.752 ms/request, but since this is was not reproducible, I omitted that.

2. Best of ten runs (ab -c1 -n100 http://domain/menu-bench, PHP 5.5.11, OpCache enabled, XDebug disabled, XHProf disabled, APCu enabled, APC class loader enabled):
   1. HEAD: 144.185 ms/request
      

                      min  mean[+/-sd] median   max
        Connect:        0    0   0.0      0       0
        Processing:   131  144  12.9    139     197
        Waiting:      120  133  12.1    128     184
        Total:        131  144  12.9    139     197
      
        Percentage of the requests served within a certain time (ms)
          50%    139
          66%    142
          75%    149
          80%    157
          90%    165
          95%    172
          98%    178
          99%    197
         100%    197 (longest request)
      

   2. patch (#2301317-6: MenuLinkNG part4: Conversion): 185.465 ms/request
      

                      min  mean[+/-sd] median   max
        Connect:        0    0   0.0      0       0
        Processing:   163  185  18.2    178     241
        Waiting:      151  173  17.5    166     226
        Total:        163  185  18.2    178     241
      
        Percentage of the requests served within a certain time (ms)
          50%    178
          66%    190
          75%    195
          80%    203
          90%    215
          95%    223
          98%    238
          99%    241
         100%    241 (longest request)
      

   This is a 29% regression as well. Since it yields the same result, I'll continue testing with this environment: everything below is tested with APCu enabled and the APC class loader enabled.

Step 2: profiling a response that does not do anything menu-related

However, let's first make sure this is not because part 4 has not yet removed dead code. In other words: let's take a look at the same benchmark (100 requests, single concurrency, fastest out of 3) when we request /system/ajax:

1. HEAD: 41.823 ms/request
   

                   min  mean[+/-sd] median   max
     Connect:        0    0   0.0      0       0
     Processing:    37   42   3.7     41      54
     Waiting:       33   37   3.4     37      48
     Total:         37   42   3.7     41      54
   
     Percentage of the requests served within a certain time (ms)
       50%     41
       66%     42
       75%     44
       80%     45
       90%     47
       95%     49
       98%     53
       99%     54
      100%     54 (longest request)
   

2. patch (#2301317-6: MenuLinkNG part4: Conversion): 46.060 ms/request
   

                   min  mean[+/-sd] median   max
     Connect:        0    0   0.0      0       0
     Processing:    39   46   5.3     45      64
     Waiting:       34   41   4.7     40      57
     Total:         39   46   5.3     45      65
   
     Percentage of the requests served within a certain time (ms)
       50%     45
       66%     48
       75%     49
       80%     50
       90%     53
       95%     57
       98%     63
       99%     65
      100%     65 (longest request)
   

This is a 10% regression. In absolute numbers, it's small, but it's consistently reproducible.

While this of course does not represent the full regression we saw above, nor is it a representative test, it is indicative of a problem that lies somewhere else than menu rendering; somewhere lower. Since the system.ajax route does not even trigger HTML page rendering, I can't help but wonder how this is possible. Can this be attributed to the increased number of services?

So I did some profiling, doing 10 runs of each (HEAD & patch) and selecting the fastest run. Looking at the profiling results, I keep seeing the class loader and DIC. So I suspect that the increase in function calls and corresponding average increase in page generation time can be attributed completely to the increase in number of services.

In other words: everything indicates that this shift in the page generation time (in the wrong direction) is primarily caused by additional I/O and secondarily by additional function calls, both of which are direct consequences of the increase in the number of services.

In any case, this is pretty much a dead end for profiling the menu link homestretch patches' impact; but it's useful to keep track of, because it does explain a small part of the regression. It explains several milliseconds.

Further investigation

I investigated further and the good news that I've now got the full explanation. The bad news is that it's probably not easy to fix.

1. Just adding services does not have a performance impact.
2. Parameter conversion services are always loaded, hence this new service does have an impact:
   

   paramconverter.menu_link:
     class: Drupal\Core\ParamConverter\MenuLinkPluginConverter
     tags:
       - { name: paramconverter }
     arguments: ['@plugin.manager.menu.link']
   

3. Event subscriber services also are always loaded, hence this service does now have an impact due to its added dependency (this used to call procedural code, which is why it wasn't a problem before) on a newly added service:
   

     router.rebuild_subscriber:
       class: Drupal\Core\EventSubscriber\RoutebuildSubscriber
       arguments: ['@router.builder', '@lock', '@plugin.manager.menu.link']
       tags:
         - { name: event_subscriber }
   

   To make matters worse, @plugin.manager.menu.link in turn depends on two newly added services: @menu.tree_storage (which in turn depends on the newly added @cache.menu) and @menu_link.static.overrides.

Together, points 2 and 3 explain the ~4ms regression that we saw in the benchmarks. If I comment them, the number of function calls with the patch drops below that of HEAD.

To solve this out-of-scope problem, I think Symfony's lazy services may be a good option — see #1973618-27: DIC: Lazy instantiation of service dependencies (ProxyManager for "proxy services").

Step 3: profiling /menu-bench

Not having gotten useful clues from step 2, I started doing general profiling of the /menu-bench response. I did 10 runs of each again and selected the fastest run.

Now, the difference has become more pronounced: I'm seeing a ~20% increase in function calls and a ~25% increase in page generation time. So it's indeed worse than what was reported by effulgentsia at #2256521-73: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module (15%).

This is not news though, effulgentsia already said this at #2256521-125: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module: there was a ~25% increase in page generation time in his tests as well (but, sadly, he only measured using ab, which as I've explained above, can result in highly varying numbers — I'm finding 29% in this report, but only because I tried to find the fastest possible run — if I didn't do that, it could just as well have been 25% or 20%).

Optimization 1: effulgentsia's patch to compensate for expensive link generation

However, at #2256521-73, by applying #2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache, he gained parity, and at #2256521-125, by applying that same patch, he found a 10% regression. Let's see if I can reproduce that as well:

As you can see, I can't quite reproduce this. There's a ~10% (8.5%) increase in function calls, but there's a ~20% (19.2%) increase in page generation time — significantly more than the 10% he reported. But, again, that might be because that was measured using ab.

(I didn't test with ab here because normally, ab & XHProf results should at least be in the same ballpark, which is not the case here. So I didn't bother.)

Optimization 2: menu block caching

One of the things I contributed to this patch, is that menu blocks (SystemMenuBlock) are no longer cached (because right now, in HEAD, it's incorrectly caching menu blocks per role, hence e.g. entity access checks are not running at all, which yields incorrect results). That correction is definitely going to contribute to that ~20% regression in page generation time. So if I manually enable caching for the Tools and Footer menu blocks and test again, I should see a sharp difference — both in profiling and in benchmarking.

Best of ten runs (ab -c1 -n100 http://domain/menu-bench, PHP 5.5.11, OpCache enabled, XDebug disabled, XHProf disabled, APCu enabled, APC class loader enabled):

1. HEAD: 144.185 ms/request (from above)
2. patch (#2301317-6: MenuLinkNG part4: Conversion) + #2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache + cached menu blocks: 155.142 ms/request (down from 185.465 ms/request, a 20% reduction)
   

     Time per request:       155.142 [ms] (mean)
     Time per request:       155.142 [ms] (mean, across all concurrent requests)
     Transfer rate:          119.70 [Kbytes/sec] received
   
     Connection Times (ms)
                   min  mean[+/-sd] median   max
     Connect:        0    0   0.0      0       0
     Processing:   135  155  15.1    148     199
     Waiting:      124  144  14.3    138     185
     Total:        135  155  15.1    148     199
   
     Percentage of the requests served within a certain time (ms)
       50%    148
       66%    156
       75%    168
       80%    170
       90%    183
       95%    187
       98%    194
       99%    199
      100%    199 (longest request)
   

So, benchmarking-wise, instead of a 29% regression, we're now looking at an ~8% (7.6%) regression. And profiling-wise it's only a 2.9% increase in function calls and 8.6% increase in wall time.

Now we can finally look at the interesting/hard part of figuring out why this is the case.

Important intermezzo

But before doing so, I'd like to call out that the extra cost that I added, to add the abstractions necessary to make #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees possible, will make it so that 1) the primary and secondary menus will be cached as well (they're not blocks currently), 2) instead of HEAD's behavior, which is for menu rendering time to scale linearly with the number of menu links, we will get (amortized) constant rendering time. If I simulate this roughly (by commenting the rendering of the primary and secondary menus in theme.inc and common.inc), then I get a benchmark result of 139.749 ms/request, which is 3% faster than HEAD.
(If that 3% improvement disappoints you: remember, that 3% will only become bigger as you end up with more complex menus — I turned a 8% regression into a 3% improvement with a primary menu that only contains the "Home" menu link and a secondary menu that only contains "My account" and "Log out" links! If anything, this quite devastatingly shows how slow menu rendering is, even when none of your menu links needs entity access checks.)

Optimization 3: Building the typical menu tree parameters is no longer cached

It was deemed a premature optimization to cache the results of MenuLinkTree::getCurrentRouteMenuTreeParameters() (which MenuTree::buildPageData() does in HEAD), but it seems this might be worth it after all.

Having applied the attached paramscache-do-not-test.patch, this is what we get…

Best of ten runs (ab -c1 -n100 http://domain/menu-bench, PHP 5.5.11, OpCache enabled, XDebug disabled, XHProf disabled, APCu enabled, APC class loader enabled):

1. HEAD: 144.185 ms/request (from above)
2. patch (#2301317-6: MenuLinkNG part4: Conversion) + #2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache + cached menu blocks + paramscache-do-not-test.patch: 151.198 ms/request (down from 185.465 ms/request, a 23% reduction)
   

                   min  mean[+/-sd] median   max
     Connect:        0    0   0.0      0       0
     Processing:   132  151  16.8    144     209
     Waiting:      122  140  15.7    134     197
     Total:        132  151  16.8    144     209
   
     Percentage of the requests served within a certain time (ms)
       50%    144
       66%    150
       75%    155
       80%    165
       90%    181
       95%    188
       98%    202
       99%    209
      100%    209 (longest request)
   

So, benchmarking-wise, instead of a 29% regression, we're now looking at an ~5% regression. And profiling-wise we now have a very slight decrease in the number of function calls (-9 calls), and only a 3.6% increase is left in wall time!

Observation: menu tree loading/transforming/rendering has become slightly slower

In HEAD, the inclusive wall time of MenuTree::renderMenu() encompasses the parameter building and loading/transform/rendering. The profiled result took 41.4 ms.

With the patch and the above 3 optimization, the inclusive wall time of MenuLinkTree::getCurrentRouteMenuTreeParameters() plus MenuLinkTree::(load|transform|build)() took 45.1 ms.

So this became about 10% slower — quite likely due to the increased abstraction, which we did both for maintainability as well as to allow for smarter caching to be added in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

Since we made the overall rendering of menus 10% slower, any double rendering of menus is going to affect us twice as much.

And unfortunately, we *do* have double rendering of menus: the incredibly silly/awful hoops we have to jump through to get cache tags for the primary and secondary menus set in HEAD require us to render those menus twice. #1805054 already fixes that, but if we "fix" that in an ugly way for now, to better approximate what #1805054 will bring us, by introducing static caching in menu_navigation_links() to essentially statically cache the rendered primary/secondary menus (navlinks_stupidity-do-not-test.patch), then this is what we get…

Best of ten runs (ab -c1 -n100 http://domain/menu-bench, PHP 5.5.11, OpCache enabled, XDebug disabled, XHProf disabled, APCu enabled, APC class loader enabled):

1. HEAD: 144.185 ms/request (from above)
2. patch (#2301317-6: MenuLinkNG part4: Conversion) + #2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache + cached menu blocks + paramscache-do-not-test.patch + navlinks_stupidity-do-not-test.patch: 148.238 ms/request (down from 185.465 ms/request, a 25% reduction)
   

                   min  mean[+/-sd] median   max
     Connect:        0    0   0.0      0       0
     Processing:   133  148  11.0    145     182
     Waiting:      123  137  10.2    133     168
     Total:        133  148  11.0    145     182
   
     Percentage of the requests served within a certain time (ms)
       50%    145
       66%    149
       75%    155
       80%    157
       90%    165
       95%    171
       98%    180
       99%    182
      100%    182 (longest request)
   

So, benchmarking-wise, instead of a 29% regression, we're now looking at a 2.8% regression. And profiling-wise we now have decreases in both function calls (-1.2%) and wall time (-0.2%)!

Combined with the observed improvement in Important intermezzo section above, I think it's obvious that it will indeed be quite easy to compensate for the minor regression introduced here.

I'm having trouble parsing this in full, but when we have render caching we can remove the use of the @cache.menu service.

We should also figure out how to lazy fetch services like @menu_link.static.overrides this is only used on rebuild, so we don't need the instance on 99.9% of page loads, so let's give attention to #1973618: DIC: Lazy instantiation of service dependencies (ProxyManager for "proxy services") soon.

I thinnk the overall message is that we need to move forward so we can achieve render caching?

The actionable steps are under Overall page generation time regression is surmountable.

But yes, we can move forward, but we must include the caching of tree parameters (see paramscache-do-not-test.patch for a rough version). And we could choose to re-enable caching of menu blocks to have better performance until #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees lands.

I have a hard time following @WimLeers' comment as I cannot find the referenced benchmark by @effulgentsia and, thus, do not know what /menu-bench does. Could someone help? Thanks! :-)

#10: Sorry — /menu-bench is provided by the benchmarking module that effulgentsia provided over at #2256521-125: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module.

(I wrote so much, yet still failed at making every detail clear :()

Part of the problems I found in "step 2" in my profiling above can apparently be solved by splitting RouterRebuildSubscriber into two parts (thanks to dawehner & catch in IRC!).

Apparently, event subscribers are only instantiated if the event they're subscribing to is actually fired. But because the onKernelTerminate event happens on every request, RouterRebuildSubscriber is always instantiated, plus its dependencies, which this series of patches has made more expensive by adding MenuLinkManager (which in turn depends on another cascade of services).

However, MenuLinkManager is only needed for the onRouterRebuild event. So we could split that off into a separate event subscriber, which means that big cascade of services will only need to happen when it's actually necessary — et voilà, part of the performance problem would be fixed :)

Here's a patch with recent changes from part3 and core

some small form fixes and file renames

here's the increment

Silly testbot - had a sporadic fail on BrokenSetUpTest because it ran out of memory or something. retest passed.

Here's a patch incorporating the 2 changes from @effulgentsia, both as full patch and only part4

New set of patches will a small change to menu_ui.admin.inc that was incorrectly in part3 instead of here.

re-rolled against 8.x, and moved one new class to part 3.

And, here's the part4 patch now that part3 is committed, with some smalls cleanups since the last one from me and YesCT.

dawehner, pwolanin and I pushed a bunch more commits. Mostly clean-up, but also to apply some of the conclusions from the profiling. We:

1. restored caching of the SystemMenuBlock
2. added caching to MenuLinkTree::getCurrentRouteMenuTreeParameters()
3. split up RouterRebuildSubscriber into two parts, to remove the added overhead on requests that don't even touch menus

#16.2: interesting! My computer definitely is overdue for replacement; I'm waiting for my computer a lot these days.
#16.3: I agree, it's concerning that our numbers are so vastly different. A third person should do profiling to hopefully figure out what's going on. Note that I'm on PHP 5.5, with OpCache, APCu and the APC class loader. Perhaps a difference in PHP execution environment helps explain this discrepancy?
#16.4: So… are you suggesting we do profiling with different situations as well? In the profiling I did at #1805054-97: Cache localized, access filtered, URL resolved, and rendered menu trees, I compared 6 different situations, which I think might be more representative of typical Drupal sites.
#16.5: That's already fixed now :)
#16.6: (What I'm about to say is slightly off-topic, but at the same time related to this series of patches, and something we must keep in mind.) I think that — roughly speaking — it's unfair to expect Drupal 8 core to be optimized for sites that have "very dynamic menu needs". Because that implies lower cache hit ratios. It's impossible to not have lower cache hit ratios. However, if they have such needs, then it's actually quite easy to fix their performance problems: change the configuration of the optimized access checking tree manipulator of #1805054 to not only cache per role, but cache per role *and* group *and* region (or whatever the actual dimensions are by which they need to cache). They'd only need to ensure they have sufficient room for additional cache entries. And yes, the individual cache entries would each be used less often, but it would mean that again fewer access checkers need to run on each page load, since access checkers that vary by group or region (in my example) will also be cached. In essence, this is horizontal scaling, where ultimately you could end up at caching per user… which is what the most complex sites out there (like Facebook) do.

So, time for another round of profiling (superficial this time), to see where we're currently at. Again applying the out-of-scope optimization (#2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache) to get an apples-to-apples comparison. Tested with ef4e932d716742952a72a5ef23d538f65fcdbf78. And the same environment as in #7 (with the APC class loader).

Benchmarking

Fastest out of ten times ab -c 1 -n100:

1. HEAD: 133.066 ms/request (versus 144.185 in #7, so HEAD became 8.4% faster since last time I tested — wow! Do we know which patch did that?)
2. HEAD + #2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache + attached patch: 128.675 ms/request

Or… a 3.5% improvement!

Unfortunately, there's one big caveat: one run with the patch was 128 ms, another was 129, but most were 134 ms and a few even 139/140. For HEAD, it was *always* 133, 134 or 135 on average.
Together with #16.2, I suspect part of the variance is due to my computer's hardware being weak/fragile (I did these tests with only Chrome, Terminal and Finder open, plus of course my web server stack — and still such variance!).
But in any case, we're roughly "on-par" performance-wise! It could be a consistent improvement, if my computer wasn't so stupid.

Profiling

Here we can see again that HEAD improved its memory consumption significantly: from 16.6 MB to 14.1 MB. But that improvement was made somewhere else than in the menu system, hence the 5% improvement we saw in #7 has now become a 7% memory improvement. Instead of 9 function calls less, we're now at 81 function calls less, so it's a 0.1% improvement now. But sadly, we still have a 5% regression in inclusive wall time… though — again — this could be attributed to my computer being too slow/overtaxed.

I forgot to attach the corresponding XHProf runs.

Note that in #7, we saw a ~0.8 MB memory improvement. But in #27, we see a ~1 MB memory improvement. I didn't verify this, but this is probably thanks to dawehner having split up RouterRebuildSubscriber :)

So, I know some work has been done on change records already, but I see no change records referenced in the sidebar of this issue or #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module. We need:

• A list of all the existing change records updates that will be needed for this patch. Each such change record should be edited to add a reference to this issue, and we should identify what will need to be changed. If it's small, we can do it after the patch, but if it's large, we should create a new draft with the new text.
• A change record specifically for the new changes, also referencing this issue.

Also, can we please get a more complete summary of what is changed in this patch, etc.? There were also several points of feedback pushed onto this issue from previous ones and I don't know if they've been addressed yet or not.

Linked the change record draft to ALL 7 issues we've been working on. It includes a list of past change records that needs to be updated, but is otherwise a bit of a stub still, and the real details should go in the handbook.

I don't think we postponed any earlier concerns except the performance fixes (done) and possibly only #2304363: Make MenuLinkTreeElement's properties protected, add getters, add limited setters, though I'd prefer to keep that as a follow-up

some doc updates needed - these are still referencing removed functions

core/modules/menu_link/src/MenuTreeInterface.php
24:   *   A data structure representing the tree as returned from menu_tree_data.

core/modules/menu_ui/src/MenuForm.php
252:   *   The menu_tree retrieved by menu_tree_data.

Thanks @pwolanin, that helps a ton.

It looks like the change record draft could use a lot of work. It previously included a list of some of the change records that need to be updated... I've transferred that here:

Here's a list of change notices that need to be updated

https://www.drupal.org/node/2240003
removed menu_get_active_trail() , menu_set_active_trail() ,menu_set_active_item($path)

https://www.drupal.org/node/2027579
The language of menus and menu links can now be configured from the user interface

https://www.drupal.org/node/1914008 Menu links have been converted to entities
D7 menu_link_load($mlid); D8 $menu_link = entity_load('menu_link', $mlid);)

https://www.drupal.org/node/1914008 Menu links have been converted to entities
hook_menu_link_alter() has been replaced by hook_menu_link_presave()

Those should reference this issue as well, since this is the issue that actually makes the changes. I'm not sure whether the list is complete... there are a lot that we should glance at and either rule out or also add:
https://www.drupal.org/list-changes/published/drupal?keywords_descriptio...

It looks like there are still a couple unresolved/unfiled @todo in the patch (at least one is a moved line of code, but others are not). I added a contributor task for checking over those as well.

Meanwhile, starting a line-by-line review.

Updated https://www.drupal.org/node/2240003 and https://www.drupal.org/node/1914008

All of these are just nits. I couldn't find anything significant to complain about :)

Yeah!!! Thank you for the intensive review.

Pushed the changes to the usual branch.

Difference between $tree and $menu_tree not clear from the variable names. Possibly a followup to improve local variable names?

Sure, here is one: #2309493: Improve $tree vs. $menu_tree variables

Is there a reason for this to be a static local variable rather than a protected instance variable?

I don't see one ... probably just c&p from the old code. I fixed this.

Here and elsewhere: when we make this change, do we still need 'localized_options', or should that be removed?

localized_options are certainly still in use for local tasks/actions. Here is a follow up to remove it in all places, if possible: #2309499: Drop 'localized_options' in all places?

Why is SafeMarkup::set() added to one of these and not the others?

You seem to just need a SafeMarkup::set() if there are multiple elements containing HTML. You don't need one if there is just one HTML string or multiple NON-html strings. Please clarify if I am wrong here. Expanded them as a form with indentation got rendered wrong.

This code is already inside a #pre_render. Why are we adding another pre_render layer rather than calling that function directly?

No idea, maybe cacheablity? wim?

Let's get a followup issue filed for this and link to it from here.

There is one: #2309501: Use plugin IDs instead of titles for toolbar classes

Doesn't look like this patch has any CSS changes included. Does this change require any?

Good question! I did a quick grep. There are several instances of ".links" selectors. Most of them are generic links, so they don't have to be changed.
I could not find any selectors which got targeted against the secondary menu or menu in general, but I could be totally wrong here.

Should "views" be capitalized?

Sure, let's do that.

Is this @todo still relevant? Looks like the linked issue is fixed, and views_menu_alter() no longer exists.

Yes, this code existed because the old menu system keyed by path, so overriding existing paths was more tricky than with the routing system, which keys by route name.

Also, we should add editing of description to both the functionality and then to this doc.

Added description, yeah let's just do it here.

If the fix for this is easy, let's add it to this patch. Otherwise, let's open a followup and add a link to the @todo.

Given that I am not even sure whether this is a problem we should have a dedicated test coverage here. Added #2309507: Ensure that menu link content/views is not loaded localized in the menu link edit form

Why do we need this check? If the plugin specified to use this form, can't we assume it implements the needed interface?

Well, this was just added for documentation purposes. Replaced it with documenting the property on the class.

How about making this $form['path']['#description'] and unsetting $form['info']?

I think it makes sense to explain the type of menu link in the exact same place. Feel free to disagree.

Any reason not to add them right now to this patch? If some of them are difficult, let's open a followup.

Done

Is there any code calling these? The implementation seems wrong to me. Since the plugin IDs are are all 'views_view:...', those if statements are never true.

Sadly I don't remember why this was maybe needed at some point. Let's drop it and add later, if it was really needed.

Since we have the {@inheritdoc}, we don't need the @return.

you are totally right.

This is a UX change/regression. For example, for the Block module, the Configure link now says "Block layout" instead of "Configure what block content appears in your site's sidebars and other regions.". I get the desire to decouple Configure links from menu links, but I wonder if this at least needs a followup issue to discuss further.

I do agree that this is a UX regression, let's also use menu links, which are even easier to use tbh. Sadly we can't rely on menu links here, so we also need the route logic. One example is the configure link of the block_content module.

At least in this patch, this seems to be a very common value for $manipulators. How about a followup to discuss making that a default for the transform() function in some way?

Sure, with #2250315: Regression: Bring back node access optimization to menu trees this list will be expanded even more. Follow up: #2309521: Provide a sane default for menu link manipulators.

Did we remove this functionality entirely for some reason or has it moved to somewhere else?

To be honest I cannot imagine that this was ever a real problem but I might be wrong.

Apparently, yes to some of the above :) Let's make sure we have an issue filed for that.

One reason these tests got dropped was that reparenting afaik this test not makes sense for content menu links.
Here is a follow up: #2309531: Add a test for re-parenting of menu links.

Added followups

This code is already inside a #pre_render. Why are we adding another pre_render layer rather than calling that function directly?

Well-spotted!

The reasons for this are:

1. Maintainability We want a similar code all across Drupal for rendering menus; the previous code was very hard to follow. Now we have toolbar_prerender_toolbar_administration_tray() which can be easily compared with e.g. SystemMenuBlock::build().
2. Cacheability We want to be able to cache all rendered menus once #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, and the toolbar's admin menu tray is a significant contributor to authenticated page load slowness. By having this in a #pre_render callback already, it'll become a simple matter of setting a #cache property next to the #pre_render property, and *poof* — it'll be cached!

Doesn't look like this patch has any CSS changes included. Does this change require any?

No, no CSS changes are necessary.

There also still are the two bugs I reported in #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module:

1. The "Home" link in the main menu in a Standard install does not get marked as active while on the front page, due to missing the value for the data-drupal-link-systempath attribute).
2. Menu links defined in YML files can be repositioned in the tree just fine. But whenever caches are cleared, the original position is restored! Or rather, this was the bug at the time of #2256521-107: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module (almost a month ago). Now, it's gotten even worse: it'll lose the repositioned menu links in the menu where it belongs, and move it to a different menu instead!
   You can reproduce this by going to admin/structure/menu/manage/admin, drag the "Structure" subtree to be at the top level, save. All is well. Now go to admin/config/development/performance clear caches, then go back to admin/structure/menu/manage/admin and … be surprised by the total absence of the "Structure" subtree… :( … and then visit admin/structure/menu/manage/tools, where they've magically reappeared. WTF! :D

The first bug described in #41 is actually a bug in UrlGenerator. Rendering menu links now uses LinkGenerator instead of l(), hence the regression.

l() does this:
$variables['options']['attributes']['data-drupal-link-system-path'] = \Drupal::service('path.alias_manager')->getPathByAlias($path);

LinkGenerator::generateFromUrl does this:
$variables['options']['attributes']['data-drupal-link-system-path'] = $url->getInternalPath();

But when rendering the front page front page link, $url->routeName == '<front>', so $url->getInternalPath() calls UrlGenerator::getPathFromRoute() which calls UrlGenerator::getInternalPathFromRoute() with the "materialized route" (i.e. /, not <front>) and then consequently fails to return <front>.

I hope the solution I went for is correct — UrlGenerator is rather confusing. It does fix the bug I reported.

And yes, I know, ideally we'd change data-drupal-link-systempath to use routes & parameters rather than system paths, but that's a much bigger undertaking, hence making that vastly out of scope.

Attached is the latest version of part4, with all the fixes that dawehner did (interdiff_dawehner.txt) to fix #36/#37/#38 and the tiny fix I did for the first bug I reported (interdiff_wim.txt). I don't know where to begin to debug the second bug I reported — hopefully pwolanin/dawehner can fix that.

The bug with the rebuild seems to be the link getting moved to the tools menu

@effulgentsia is doing the line-by-line. :)

Haven't resolved the shortcut fail, but let's see if the bot is otherwise happy.

@effulgentsia
Please also push the changes into the repo.

I re-created the repo branch from Alex's patch plus added my menu name fix

Here's patch + interdiff.

The added foreach doesn't appear useful.

Yeah, no idea how this happened.

$link doesn't appear in the code below this.

Renamed to $entity to make it useful.

Would 'menu_link_definition' be a clearer key name?

Agreed and changed

Although $node->menu is a preexisting condition of HEAD, can we open an issue and add an @todo here to remove that? In D8, we shouldn't be adding arbitrary non-field properties to content entities.

Opened a follow up: #2310173: Don't add arbitrary non-field menu to $node

Preexisting condition of HEAD, but I'm confused by this comment and @todo. I think it's made slightly worse with the conversion to use the selector, because I don't think it's intending to imply that some change is needed to that selector method itself.

Should we open just another follow up?

Do we need the "_plugin" suffix? Now that the conversions are done, wouldn't {menu_link} be unambiguously the plugin?

Just wondering whether _plugin makes it easier to disinct between an ordinary menu link and a menu_link_content

Move $manipulators lower so it doesn't separate the code comment above from what it refers to.

Good catch!

Why did we rename the form field to "enabled" if the definition key is named "hidden"?

We do want to have enabled/status at the end everywhere because it is easier to get. The node menu form got rewritten so we just did that.
On top of that the UI was always with an enabled checkbox.

s/and item language//

Wow!

Why are we verifying this by visiting the node page. Is the link not showing up on the front page for some reason?

It still passes without it ... i guess this was just needed temporarily.

So with the new architecture, we're losing this feature. I think that's ok, but we need to make sure that's documented somewhere (I guess in an 8->8 change record?). Also, menu_link_content module in its current form doesn't have a UI for managing bundles. Are we punting that to contrib, or plan on adding that in a core followup?

We decided to move this to contrib as you might want to have bundle per menu or not, depending on your usecase of fields.

Looks like this was the only place testing the /reset route via the UI. Can we add back a test that does that?

Will work on that later during the weekend

is the itemized list in the proposed resolution in the summary an overview of all the parts so far 1-4, or just the changes in part 4? which should it be?
--
saving this again since my first save was lost...

Tabulating responses to each of the review points from @effulgentsia at https://docs.google.com/spreadsheets/d/1EPoJkSOG8R0NmFlY7adVXaoZgPvKudSA...

re: #49.7: The menu link content module creates routes with paths like: /admin/structure/menu/item/{menu_link_content}/edit
so, I guess it could be clear that {menu_link} -> MenuLinkInterface, but maybe we can discuss in a follow-up? Also, in this step we still have other routes that load {menu_link} as the old entity.

Also re: #49.9 there is already a follow-up to fix this to a status field which is referenced in the @todo in core/modules/menu_link_content/src/Entity/MenuLinkContent.php. #2305707: MenuLinkContent disabled field should be renamed to enabled and use positive logic

Also added a missing hook_help as pointed out by @tim.plunkett in IRC.

re: #53.5 dawehner is working on adding test coverage. I think all other review points have been addressed.

re: #49.6 added a follow-up at https://www.drupal.org/node/2310319 and re-worked the code comment to include a link to that.

re: #53.5 this also has dawehner's added test for reset functionality.

Better fix for the active class - handles it in the LinkGenerator and restores the test. plus a bunch of follow-up links added to @todos by YesCT.

(last local commit: c6092ed7c7036e4)

There are no JS changes relative to 8.x now.

todo's all have issues. updated remaining tasks and summary in the summary.

html.

Silly me - the ternary in LinkGenerator was wrong except on the front page. 1 line fix, should fix all those fails.

Also fixed ViewsMenuLinkForm per #67 and doc and test fixes from YesCT, and made the MenuLinkBase more clearly abstract by moving the implementations of getTitle() and getDescription() to MenuLinkDefault. That change due to trying to write up developer examples and realizing that would be a better pattern.

@effulgentsia - Gabor reviewed these changes in terms of multilingual a while back including that change. Using the menu name as the bundle makes absolutely no sense for a filedable entity - it's just not supportable. The text from that issue is basically not really relevant given the changes we are making. If you want a menu full of non-translated links you'll have lots of options to achieve that.

This change record needs to be updated still: https://www.drupal.org/node/2226481

+++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
@@ -40,18 +40,7 @@ public function getWeight() {
+  abstract public function getTitle();

@@ -112,12 +101,7 @@ public function isDeletable() {
+  abstract public function getDescription();

@@ -190,6 +174,11 @@ public function getTranslateRoute() {
+  abstract public function updateLink(array $new_definition_values, $persist);

You can just delete these. The class is abstract, and these methods are on the interface.

@tim.plunkett - we were debating that question in IRC. I think it's better to have them there to document that they need to be implemented, and we were not sure we have a clear coding standard.

when to use abstract:

1) protected methods
2a) public methods on traits
2b) public methods on classes with no interface [bad!]

There is no reason to use it when its backed by an interface and its public. We don't have these anywhere I could find, no reason to introduce them here.

I finished reading through the patch. (Did *not* read every line of every test carefully.) Fixed a bunch of nits, docs, namings, and a few typehints in the sandbox.. and addressed #71. I guess this is ready for a new patch and interdiff when @pwolanin gets a chance.

On to manual testing.

0a. These could all be addressed in (multilingual) follow-ups... (in part because for each thing that does need work, it will need a test.
0b. do not let #2188675: "Translate" local task always visible, leads to WSOD distract from this issue. To test translation, enable content translation. (with that bug, you will see translate tabs even though you should not)

1. Problem:
The translate tab/page for custom menu links is not in the admin theme. Maybe we can do something similar to #2276387: Translate routes should properly inherit admin path use from edit route here or in a follow-up.

2. Translations
2a. As expected:
plugin/discoverable menu links are translated using User interface translation (locale, po files, po overrides)
2b. As expected:
custom menu links (menu link content entities) are translated by installing the content translation module.
2c Concern:
Where can we add to the help that for Menu links (not Custom menu links), use the User interface translation?

3. (unrelated, nothing wrong, but noting) There is a language select on menus (menus are config) (when language module is installed):

4. Language of menu links...
4a. Working as expected:
There is a language default select on the content language overview for custom menu links

4b. Working as expected:
there is no language select on individual menu links until Show language selector on create and edit pages is selected on the content language settings page for custom menu links.
4c. Problem:
Creating a menu link, then showing the language selector, and then editing the menu link and selecting a specific language
[which is effectively changing the language of a custom menu link from not specified to en]
gives:
"Site-Install
Error
The website has encountered an error. Please try again later."
and does not save the language of the menu link.
Not great.
4d. [edit] Problem Works as expected:
Creating a menu link when there is a default language for custom menu links (for example en), works ok. Changing the language (to say af) does save that menu link as af.

5a. Working as expected
enabling configuration translation makes *menus* have a translate operation. (No need to enable translation for menus, all config is translatable once config translation module is installed).

6. Turning on content translation
6a. Problem:
unsets the language select defaults for custom menu links. :(
(for example, the show language selector got turned off)

7. Translating custom menu link has several nice things.

7a. page title like "Create English translation of Two af" just works and gives nice context.
7b. setting certain fields as not translatable (for example description), gets the (All languages) on the form.

8. Problem
editing a custom menu link and unchecking show as expanded, does not save. Reloading the edit page still has show as expanded checked.

10. Custom menu link module help (menu_link_content_help)

10a. I think we need to take out the bit about translation (because multiple languages is not (should not be) enough to translate once #2188675: "Translate" local task always visible, leads to WSOD is fixed).
10b. "it" reads a bit awkward and ambiguous.
10c. Maybe some paragraph tags to split up ideas would help.

11. Works as expected:
Reordering menu links in a menu

12. Works as expected
Moving a menu link to another menu

13. Works as expected
Reseting a (plugin/config/non-content/non-custom) menu link.

14a. Concern
if a custom menu link was created before language select was set, then later translation was enabled, the delete form is a bit odd:

14b. Works as expected
Delete form for a translation made after turning on language and translation is fine.

15. Concern
Custom menu link that is translated is not showing translated when the path of the page displayed is in that language
(in my test set up, Four af, should have been shown as the menu title on a af page)

100. I had thought that language select on nodes (or whatever) was typically last on the edit form, but that is not the case. For nodes, language select is after title and before body. For custom menu links it is after: title, path, description, and before the enable checkbox.

101. Possibly unrelated problems
101a. deleting the translation of an article get the website encountered error.
101b. editing a translation of a node gets same error.

-----
Summary.
Should try these again, with definite steps to reproduce.

Thank you for that late manual testing!

The translate tab/page for custom menu links is not in the admin theme. Maybe we can do something similar to #2276387: Translate routes should properly inherit admin path use from edit route here or in a follow-up.

This is not a follow up: #2310475: Admin theme for translation tabs just works for nodes

Where can we add to the help that for Menu links (not Custom menu links), use the User interface translation?

Question: How do we do that for nodes?

Creating a menu link, then showing the language selector, and then editing the menu link and selecting a specific language
[which is effectively changing the language of a custom menu link from not specified to en]
gives:
"Site-Install
Error
The website has encountered an error. Please try again later."
and does not save the language of the menu link.

Please ALWAYS show errors (configured on admin/config/development/logging). Sadly drupal does not do that any longer by default.

I tried the same and it worked fine. I used two languages: en and fr (both content translation installed and uninstalled).

4d. does not sound like a problem, right?

6a. Problem:
unsets the language select defaults for custom menu links. :(
(for example, the show language selector got turned off)

This sounds like a content translation bug, doesn't it?

editing a custom menu link and unchecking show as expanded, does not save. Reloading the edit page still has show as expanded checked.

Note: The checkbox should be marked as disabled given that it is for "all languages". Is there an issue for content translation already
to do that?

if a custom menu link was created before language select was set, then later translation was enabled, the delete form is a bit odd:

Isn't that the same for node?

Custom menu link that is translated is not showing translated when the path of the page displayed is in that language
(in my test set up, Four af, should have been shown as the menu title on a af page)

Fixed that, ... this was just a caching bug.

... multipost

new patch with fixes from dawehner and YesCT for the issues they and tim,plunkett found, plus flags the menu link content edit route as admin which I think will resolve the issue YesCT found of it not being in the admin theme (seems to work for me locally).

Thanks for the reminder to always show errors (admin/config/development/logging). I had looked in the apache log, but forgot about that new-ish setting.

#75
1. fixed. translate tab in non-admin theme.
2c. Creating separate issue. Where to document the 3 different ways of translating menu things. Config translation for menus. User interface translation for default menu links. Content translation for Custom menu links. #2310639: Document (expose) how to enable translation for Menu Things (Menus/Configuration translation, (Plugin) menu links/User interface translation, Custom menu links/Content translation)
4c. Having trouble reproducing this one. But was able to get an error another way: Enable content translation for Custom menu links. Create a custom menu link. Add a new language. Go to the translate page for that menu link. Click add to add a translation for that new language. Error is "invalidArgumentException: Invalid translation language (fi) specified". Issue for that: #2310647: Allowed language langcode argument cached cause Error
6a. could not reproduce
8. - still a problem. Enable content translation for Custom menu links. Add a custom menu link, leave show as expanded unchecked. Save. Edit. Show as expanded is checked.
10. can be done later as part of 2310639 or a different separate issue.
14a. could not reproduce the strange delete confirmation page
15. I'll be back in a bit to verify that is fixed.

15. verify that it is fixed. custom menu links are showing the translated titles both in the navigation and in blocks in the sidebar.

For the interested, the fix was:

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -131,7 +131,7 @@ public function getCacheTags() {
   protected function getRequiredCacheContexts() {
     // Menu blocks must be cached per role: different roles may have access to
     // different menu links.
-    return array('cache_context.user.roles');
+    return array('cache_context.user.roles', 'cache_context.language');

I can also verify that selecting the source for the translation works the same as for nodes.

What is a bit different is the author and date. in Menu links it is in the translation collapsed field set, and in nodes it is it's own field set (on the side/in the vertical tabs)
#2310657: Properties/Fields like author and date for Custom menu links translations

marking manual testing as done.

8. fixed in the sandbox with

+++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
@@ -211,7 +211,7 @@ protected function extractUrl($url) {
    */
   public function extractFormValues(array &$form, array &$form_state) {
     $new_definition = array();
-    $new_definition['expanded'] = !empty($form_state['values']['expanded']) ? 1 : 0;
+    $new_definition['expanded'] = !empty($form_state['values']['expanded']['value']) ? 1 : 0;

Fixed the expanded problem and added a TODO

Looks like #83 could use explicit test coverage but also like it broke something else? :)

dawehner and I worked through this bug - turn out the fix he made for expanded form values broke the test since all links had been getting expanded before and hence appearing on the home page unexpectedly. This is why we were checking the $path instead of home page in earlier version, but changing that from $path back to '' in response to #53 + the actual form values fix, created a test bug.

Easiest fix is to go back to marking the parent links expanded. Also fixes up some confusing code comments.

I'm good with the feedback and manual testing results.
child issues are good to cover things
#2310475: Admin theme for translation tabs just works for nodes can add tests for the admin theme on translate link situation as it will also take out the fix stuck in here.
also added
#2311449: Add tests for menu links not expanded on front page and other pages
#2311451: Add test for menu links and custom menu links are rendered translated in both nav menu and a menu block
just to make sure.

re-editing the issue summary to re-mark manual testing task as done

so. I'm good.
next, pwolanin and I are working on the change records

clarifying in the summary.

trivial doc fix

change record list added to the issue summary with link to the revisions to review. (the revisions links would need updated as further changes to them are made that this issue missed so far)

[edit] ug. sorry about the file removed from the recent files table.

noting that a complete search was done to find all change records that need an update.

see #91 for latest patch with passing test run at https://qa.drupal.org/pifr/test/833188

cross-post seems to have confused d.o

I'm good with YesCT's followups. I'd still like to confirm that all the dangling points from previous issues have been addressed or had their own followups, but I will... follow up with that later.

Ideally alexpott would review this since he reviewed the others and had some of those previous bits of feedback... (sorry Alex).

Also adding myself to the commit message as I spent an entire day trawling through CRs (with help from dawhener, pwolanin, and effulgentsia) and I'm still not done. But okay with RTBC on the assumption that this patch takes a bit to marinate and review anyway (and because I've resigned to doing the needed changes myself).

Oops I broke the patch... rerolled.

Nice try, bub. ;)

@xjm: I actually think it would be great if catch takes this one - more eyes! I still plan to have a good look at the patch. Chatted with catch already and he seems okay with this plan.

Assuming this is just continued crossposting and not "screw you, xjm, no one values your work"... :P

8.0.x please

Update yer bookmarks!

Having two change records entitled "Menu links converted to plugins..." was super confusing, so I've incorporated the old one into the new one:
https://www.drupal.org/node/2302069

We can delete the old one (https://www.drupal.org/node/1914008) after the new one is published.

Still need to document the upgrade for a couple more constants, and then improve the introduction/overview a bit.

Overall this looks great. One nit-pick, one real question. Didn't see anything else that would hold up commit, everything that looked a bit off has a @todo and issue linked.

1. +++ b/core/includes/menu.inc
   @@ -367,21 +405,40 @@ function _menu_link_translate(&$item) {
   +  if (!isset($variables['attributes']['class'])) {
   

   Very minor but $variables['attributes']['class'][] = 'menu' should just work whether it's set or not without throwing a notice.

2. +++ b/core/modules/menu_link_content/menu_link_content.install
   @@ -0,0 +1,27 @@
   +  // Find all the entities and then call the manager and delete all the plugins.
   

   Really? Won't that be OOM on lots of sites?

Opened #2312389: Remove menu_link_content_uninstall().

Committed/pushed to 8.0.x, thanks!

Published https://www.drupal.org/node/2302069 and deleted the old one.

Huzzah! Congrats to everyone who's been pushing this epic issue forward for so long. :-)

I reverted and re-committed to bring the commit credits up to date with the suggested commit message in the summary.

Hurray!

Onwards to #2301319: MenuLinkNG part5: Remove dead code; and party! now, and then we'll finally be able to close #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module, at which point only 3 beta blockers will remain!

re: #106 - we are not deleting the entities, just the plugins, but yes - uninstalling menu link content on a site with larges abounts of that entity would be slow or OOM - but are people going to do that?

Also, _menu_link_translate() is deleted in part5, so was that just showing as context?

Hey gang, tested HEAD on Simplytest.me in order to see if I could find the UI changes. Found a bug instead. Think it might be this patch that introduced the regresssion:

@cosmicdreams, that looks actually like a double-escaping issue from the Twig autoescape change. In any case, please post a separate issue with steps to reproduce, as a child issue of #2297711: Fix HTML escaping due to Twig autoescape.

Tagging the child issues retroactively.

This caused #2322933: Module list form fatals due to incorrect route configuration.