#473268: D7UX: Put edit links on everything introduced a relatively large performance hit, even for anonymous users.

I already worked hard to decrease the actual processing time in http://api.drupal.org/api/search/7/menu_contextual_links (though not sure whether everything in there can be kept as is, because, f.e. a $parent_path "node" would have to account for additional node access logic... and that could equally apply to many other objects with custom access handling -- but there is no facility in core, which provides a list of such things)

So I think what I want to try next is to make menu_contextual_links() a simple static cache of the passed in arguments, and perhaps move the retrieval/building/rendering to a central facility.

chx additionally asked for a global killswitch. We can additionally do that, but let me add that the intention of these links is to also move mission critical administrative/moderation functionality into contextual links, such as comment moderation links.

Quite a bit to tweak here. :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Subscribing.

effulgentsia’s picture

This is a follow-up comment to my initial report in http://drupal.org/node/473268#comment-2161218 and Jacob's questions in http://drupal.org/node/473268#comment-2162520.

Yes, absolutely, standardized benchmarking methodology on a standardized production machine needs to be done -- ideally, I'd love to see an automated benchmarking suite run on every submitted patch for the D8 dev cycle. Just like testbot has been incredibly useful to the D7 cycle, how cool would it be to have a benchbot for the D8 cycle!

My result of a 6% slow-down was from a MAMP machine running PHP 5.2.6, with PHP memory limit of 256MB, APC on, xdebug off, mysql query cache off. I described the page setup I was testing on http://drupal.org/node/473268#comment-2161218. In summary, the front page with 10 node teasers and a bunch of blocks (all blocks that start off disabled after a fresh install of HEAD placed into either the left or right sidebars) served to an anonymous user. For each test, I would first "clear all caches" in /admin/config/development/performance, then run "ab -c1 -n100" on the home page twice, and use the results from the second run. The first run always has more outliers while, I presume, APC and system caches are being primed, so the second run tends to be a better measure of PHP time.

For someone wanting to generate more official numbers, you can use the above as a guideline. Basically, the goal is to test a page that can potentially have a lot of contextual links. Contextual links appear on node teasers, blocks, menus, and comments. I only tested the difference for anonymous users (for whom the links aren't rendered, because at some point the code figures out that the user doesn't have access to the destination of the link), but it might also be interesting to test it for admins (for whom the links are rendered).

Here's the easiest way to test the performance difference between running the contextual links code vs. not. HEAD has it on. Until a global killswitch is added, the way you can turn it off is by making the 1st line of menu_contextual_links() be return(array());.

Once we have official numbers, I think the two things to evaluate are:

1) how much slowdown is acceptable in exchange for the user having access to useful links (i.e., Jacob's point that a more efficient workflow compensates for slower page response) and

2) how much slowdown is acceptable for users who end up not having access to the contextual links anyway?

In regards to #2, anonymous users get cached pages anyway, so I don't think they provide a compelling use-case for #2, but what about authenticated users without permission to edit nodes or configure blocks/menus? Sun and Dries both disliked the idea of a global permission ("access contextual links") that can provide something like a per-role global kill-switch, but without that, how can we minimize the overhead of all that contextual link code, especially when it won't result in anything being rendered that would improve the user's workflow enough to have made the overhead worthwhile?

chx’s picture

There was a little bit more than asking for a performance killswitch. We have agreed on that this is going to be a performance blow no matter what because tasks are slowish and that should really be just those who see it. I even thought it got added to the original patch... or we discussed an edit mode but that was shut down.

In short, as a menu maintainer I have only agreed on using tasks because of that killswitch. I did a bit more than 'asked for'. If this would have been any sort of a normal process instead of the slush rush, it would not have gotten in without it.

sun’s picture

To clarify:

  • The additional permission was contained in an earlier patch, and even earlier patches contained a server-side mode-toggle.
  • After lengthy discussions we came to the conclusion that stuff like comment edit/delete (administrative) links could or should also be moved into contextual links, so they do not interfere with the regular comment output.
  • Overall, this opens up the question whether we can simply "disable" them. The UX team will have to take a decision on whether expanding the usage of contextual links to other areas is feasible. My personal opinion is - it is, because we are talking about an entirely new interface paradigm here and there is nothing bad in not supporting it, because it's really awesome.
  • If that is going to happen (and I'm pretty sure it will, because it's not only UX, but also a major improvement for themers), then we need to think and brainstorm about speeding up the process of retrieving and rendering them.
  • The current implementation is no longer re-using menu_local_tasks(), but instead just retrieves the first layer of local tasks registered for a certain, normalized path (like node/%).
  • Due to the normalization, contextual links are only retrieved once for a certain object (i.e. block, node, etc.) during a single request.
  • However, menu_get_item() is still invoked for each call to menu_contextual_links(), and in case we found contextual links, _menu_translate() is invoked for every single normalized item to re-inject the arguments.

Considerations:

  1. It's most probably senseless to call menu_get_item(), because we know the base/parent path already, and we especially don't need to translate the parent item, because we won't use it anyway. That may very well be the top #1 slow-down.
  2. The current approach of entirely skipping subsequent requests for the same base/parent path in case a previous one didn't return anything is wrong, because it doesn't account for stuff like node_access.
  3. If there was a difference between access-per-entity and access-per-object, then we could easily skip many access checks for many paths. (Examples: access-per-entity: blocks; access-per-object: nodes).
  4. For the retrieved router items, we only want to invoke the access callback to check access and optionally localize the title.

Let's brainstorm about solutions.

catch’s picture

FileSize
591.48 KB
420.21 KB

For a start this causes an additional 17 database queries on the front page with one node and a couple of blocks (as uid 1). I'm a bit surprised this information wasn't attached to the original issue tbh.

Attached screenshots with the devel query log for head and with return array() at the top of menu_contextual_links().

Saying that anonymous users get cached pages is a red herring by the way. They don't get cached pages when they're viewing an uncached page - and the more expensive uncached pages are to render, the more likely servers are to fall over if there's cache clear, or if a particular site has a lot of cache misses anyway.

It's also quite possible to have authenticated users without any editing permissions, particularly for blocks, but also comments, nodes etc., who'll similarly get a performance hit without any of the efficiency benefits this patch promises. Drupal 7 performance is already shocking, and we just made it considerably worse, just as the criteria for performance patches is going to become a lot more strict. Great.

sun’s picture

I guess we need an index for the new "context" column?

sun’s picture

EXPLAIN SELECT * FROM menu_router WHERE (tab_parent = 'admin/structure/menu/manage/%') AND (context <> 1) ORDER BY weight, title

id  select_type table        type  possible_keys  key         key_len  ref    rows  Extra
1   SIMPLE      menu_router  ref   tab_parent     tab_parent  767      const  3     Using where; Using filesort

Removing 'title' from ORDER BY skips the filesort. There's already an index for "tab_parent", which now needs the additional key 'context' (and perhaps 'weight', but not sure).

moshe weitzman’s picture

Subscribe ... IMO, we reached a new low. The benchmarks for this issue showed us that we had a 6% slowdown and it was still committed thinking "we will fix in follow-up issues". Well, thats OK when you are 6 months from freeze but we should be getting more strict now, not less.

Anyway, lets try to fix this stuff.

sun’s picture

Status: Active » Needs review
FileSize
973 bytes

This fixes the filesort.

I'll do some more profiling later.

catch’s picture

Bojhan explained in irc that the patch was committed without an 'edit mode' toggle of any kind, discussion of such a thing to be deferred to a followup patch which isn't around yet.

If we have an edit mode toggle then -

1. People who need these links who are browsing a site normally most of the time (like, for existence, any registered user on Drupal.org) could switch it off and avoid the hit - since we wouldn't need to generate the links if we know edit mode is off.

2. For roles with no edit permissions at all, either core or individual sites could set the toggle to default to off - again to avoid a hit for people who won't be using the links at all. If we decide against a permission, it could perhaps be a per-role setting as to the default state.

While we should do as much as possible to optimize how the links are generated, checking access callbacks in the menu system is expensive, no matter what you do, so I think there's a limit to how much can be done here. If I only get the hit in 'edit mode' though, then Jacob's efficiency assessment applies a lot better.

chx’s picture

UX can not overbid performance. I agree with Moshe. It seems we need an edit mode toggle to curb the performance hit, let's add one.

catch’s picture

Looked through the devel query log on the front page with no content, clean install, user/1.

Queries added or which look related to this:

menu_get_item() *1


SELECT menu_router.* FROM menu_router menu_router WHERE (path IN (:db_condition_placeholder_0)) ORDER BY fit DESC LIMIT 0, 1

Might be already in HEAD.

menu_get_item() *1

SELECT menu_router.* FROM menu_router menu_router WHERE (path IN (:db_condition_placeholder_0, :db_condition_placeholder_1, :db_condition_placeholder_2, :db_condition_placeholder_3, :db_condition_placeholder_4, :db_condition_placeholder_5, :db_condition_placeholder_6, :db_condition_placeholder_7, :db_condition_placeholder_8, :db_condition_placeholder_9, :db_condition_placeholder_10, :db_condition_placeholder_11, :db_condition_placeholder_12, :db_condition_placeholder_13, :db_condition_placeholder_14, :db_condition_placeholder_15, :db_condition_placeholder_16)) ORDER BY fit DESC LIMIT 0, 1

But I don't think there were two, so one must be a dup.

menu_contextual_links() *2

SELECT m.* FROM menu_router m WHERE (tab_parent = :db_condition_placeholder_0) AND (context <> :db_condition_placeholder_1) ORDER BY weight ASC, title ASC

Why is this running twice?

menu_get_item() *3

SELECT menu_router.* FROM menu_router menu_router WHERE (path IN (:db_condition_placeholder_0, :db_condition_placeholder_1, :db_condition_placeholder_2, :db_condition_placeholder_3, :db_condition_placeholder_4, :db_condition_placeholder_5, :db_condition_placeholder_6, :db_condition_placeholder_7, :db_condition_placeholder_8, :db_condition_placeholder_9, :db_condition_placeholder_10, :db_condition_placeholder_11, :db_condition_placeholder_12, :db_condition_placeholder_13, :db_condition_placeholder_14, :db_condition_placeholder_15, :db_condition_placeholder_16, :db_condition_placeholder_17, :db_condition_placeholder_18, :db_condition_placeholder_19, :db_condition_placeholder_20, :db_condition_placeholder_21, :db_condition_placeholder_22, :db_condition_placeholder_23)) ORDER BY fit DESC LIMIT 0, 1

That's now five calls to menu_get_item(), 3 running exactly the same query as far as I can see.

block_load() * 6 (once per block)

SELECT * FROM block WHERE module = :module AND delta = :delta

Possible solution to the block_load() issue would be:
a. Don't use a menu loader (menu loaders are run for every, single, menu system access check.
b. #430886: Make all blocks fieldable entities adds block_load_multiple() - this would need to happen before the menu access checks though, that way it'd be accessing a static instead of a query for every single block on the page one by one.

Note that we're also duplicating the data that's already in block_list() here. What a mess.

menu_load_links() * 1

SELECT ml.* FROM menu_links ml WHERE (ml.menu_name = :db_condition_placeholder_0) ORDER BY weight ASC

The quick wins would seem to be the block loader / access check, the duplicated menu_contextual_links() query, and I think sun mentioned that we don't really need to do menu_get_item() in all the places it's called.

I count 14 queries added here, may have missed one or two.

Then I added a node, and did the same on node/1

changes:
menu_contextual_links() does three database queries on node/1

Added a comment
changes:
The comment is loaded 5 times from the database:

SELECT base.cid AS cid, base.pid AS pid, base.nid AS nid, u.uid AS uid, base.subject AS subject, base.comment AS comment, base.hostname AS hostname, base.created AS created, base.changed AS changed, base.status AS status, base.format AS format, base.thread AS thread, base.name AS name, base.mail AS mail, base.homepage AS homepage, base.language AS language, n.type AS node_type, u.name AS registered_name, u.signature AS signature, u.picture AS picture, u.data AS data FROM comment base INNER JOIN node n ON base.nid = n.nid INNER JOIN users u ON base.uid = u.uid WHERE (base.cid IN (:db_condition_placeholder_0))

I repeat, 5 times. That's once for _load_multiple() and once for every contextual link per comment.

menu_contextual_links() now queries the database 4 times - once for blocks, once for comment, then two others.

Not sure if it's down to contextual links yet, but comment_num_replies() appears to be running once for every single comment, not sure if that was happening before or not.

catch’s picture

So let's fix some of these:

Promoted #430886: Make all blocks fieldable entities to release blocker.

Opened: #611590: Statically cache comments

Looks like menu_contextual_links() isn't a duplicate query, it's one at least once per type-of-thingy-which-can-have-contextual-links, so no luck there. However that makes sun's patch in #9 extremely important.

Will wait for sun to come back with what he was thinking about menu_get_item(), but would be good to get rid of those obviously.

While it looks like we can remove some of the queries - particularly comment and block, this is still going to add at least 6 or so queries per page which are nearly impossible to get rid of, along with an unspecified amount of time in PHP. Either way, this screams for an 'edit mode' toggle - one which can be set per-user, and also globally defaulted per-role - so that if you know you never want your anonymous or basic auth users to see these links, you can switch it off entirely for them, and also browse the site without them - imagine everyone on Drupal.org browsing with edit links on all the time by default, would be crazy, not to mention making the UX of the site worse - I might have permissions to edit blocks configuration on Drupal.org, but I never actually need to do it, so I don't need to see the links all the time.

Also let's not get into performance vs. UX - performance is an extremely important component of user experience - the longer web pages on a particular website take to load, the less likely people are to load them. At approximately 1/3-1/2 the speed of Drupal 6 for non-administrative authenticated users on standard pages, Drupal 7 has an extremely long way to go, efficiency arguments for admins don't counter that at all.

catch’s picture

Probably not a performance issue, but #611642: Roll back comment contextual links (temporarily) is just a straight bug - would save a few function calls at least.

Dries’s picture

It sounds to me like we need to make this a server-side toggle or kill-switch. Like that, the performance overhead will be zero when toggle off, or for all those people that are not allowed to use the edit in place. What is the issue with having a server-side toggle?

Bojhan’s picture

So, somewhat sad that I have to subscribe to this topic - @catch performance vs. usability thing is inrelevant, we shouldn't have to choose. Would be great not to turn this issue, into one making high impacting decisions on the UX of this feature. If it is, fine - but please notify me or yoroy then. We are trying to figure out how to really work this edit in place into our node (maybe comments) whatever actions, but its somewhat troublesome given the timeframe.

I imagine almost all CRUD links turning into edit in place, but we need to have a good fallback mechanism for things as "Report as spam" . And a good mechanism for modules to leverage this functionality.

I hope we can find a good solution.

catch’s picture

@Dries, I think we need a toggle - per-user and also a default per role, as mentioned in #10 and elsewhere. However I also think we should optimize the obvious places where this is inefficient - which is comments and blocks so far.

David_Rothstein’s picture

1. Yeah, we definitely need a toggle. For months, the patch at #473268: D7UX: Put edit links on everything had either a per-user or per-role toggle or both, but it seems like it was removed almost literally minutes before the patch was committed? - well, I guess it was a very hectic time :) In any case, some kind of toggle (exact kind or kinds to be determined) is essential for both performance and usability.

2. We still should improve performance in other ways because some sites might want to enable this feature for a significant percentage of their users. The issue with menu_get_item() is pretty simple: With contextual links, we call it for node/1, node/2, node/3, etc (all of which correspond to node/%) but the function is not able to optimize for that, which I think is why we have duplicate (or perhaps just near-duplicate?) database queries. Plus we call _menu_translate() twice, once from inside menu_get_item() and once directly in menu_contextual_links(). So there is hopefully room to improve there, either by optimizing menu_get_item() or skipping it altogether.

3. Comments are a pretty special case for a variety of reasons (both performance and user experience). I thought we planned in #473268: D7UX: Put edit links on everything that they should not be included in the initial patch so that they could be considered separately? In any case, we're discussing them separately at #611642: Roll back comment contextual links (temporarily) now anyway :) We have a variety of options there - for example, in the absolute worst case, we could easily make them appear as contextual links without taking a performance hit, by adding them manually but not using menu_contextual_links(). These kinds of ideas were discussed in detail in the previous issue.

4. Regarding block_load() in #12, those are some interesting results. Note: This function was added to core in #606640: Use proper menu router paths for the block module (but no argument loader functions) but there was no reason it needed to be added. The goal of that issue was just to rename URLs to make the contextual links work (and an earlier, simpler patch there did not introduce a block_load at all). We can easily stop using this function if it's causing problems.

5. Let's keep in mind that there were some extraordinary circumstances surrounding this patch - the ideas for changing the API came up at the very last minute, and @sun did an amazing job just putting together something that worked with the new approach (and even preemptively addressed some of the performance concerns). The performance issues inherent in this approach were mostly known several days in advance and written up in a fair amount of detail at #473268: D7UX: Put edit links on everything, specifically so that the information would be there for a decision to be made on how to proceeed... In the end, going with the best API possible was probably the right decision - this is an important feature. So the issue here is simply a continuation of the previous one, and it's (cleverly) marked as a bug so that there's plenty of time to fix it :)

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
1.3 KB
13.21 KB

So here is what we will do:

1) Fix the missing table index on {menu_router} to vastly speed up local tasks + contextual links queries.

2) Re-add the user permission that was removed right before the contextual links patch was committed.

3) Remove the contextual links from comments. However, allow contributed/custom modules to re-add them - which requires us to minimally re-purpose and fix the invocation of hook_comment_build_alter(). The same is true for hook_node_build_alter(). Not sure whether there are more $object_build functions in core that need to be fixed equally.

4) Entirely remove all function invocations for contextual links except the #pre_render function, which in turn, skips entire processing if the current user does not have access.

Attached patch accomplishes that.

sun’s picture

So I grepped over core and the only remaining instance of $object_build is user_build(), which equally invokes user_build_content(), but there was no hook_user_build_alter() yet, so for consistency reasons, this is added accordingly here.

sun’s picture

An while being there, this entire performance revamp additionally allows us to fix something I was a bit worried about when doing the original implementation.

Due to the direct invocations of menu_contextual_links(), I didn't want to stuff further function arguments into the calls. That lead to the issue that all contextual links are keyed by the actual local task path argument (i.e. "edit", "configure", "list", etc), which in turn means that one contextual link key ("edit") can only exist once in a set of contextual links. However, since we are now no longer invoking the function directly, but instead have some nice structured data array, we can use the existing data to prefix each contextual link key with the name of the implementing module.

Very trivial fix, and I only stuff this into this patch, because we're touching those lines anyway already, since the documentation of menu_contextual_links() was outdated/incomplete somehow.

.

So, yeah. This patch reverts the ~6% performance decrease for all requests/users that do not have access to contextual links, and it also improves performance for users with access (due to the added table index).

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
17.84 KB

I am unable to replicate those failures locally, and, well, DBLog functionality is totally unrelated to this patch. (?)

moshe weitzman’s picture

I looked through all the render changes and they look like a nice cleanup. If someone else could review the menu changes, I think this is ready.

sun’s picture

yeah, it would be nice to commit this, because commenting out the contextual links code wastes time for people trying to do proper performance benchmarks in other issues.

sun’s picture

Status: Needs review » Reviewed & tested by the community

meh. Since...

1) I implemented contextual links, I can approve the changes to it

2) moshe approved the $object_build[_alter]() changes

3) there are no menu system changes in this patch

4) there are benchmarks that verify the performance (re-)gain

...I'm marking this RTBC, because it really makes performance benchmarks in other issues harder than they need to be.

This patch re-introduces the user permission for contextual links, which was removed right before the initial contextual links patch was committed. If there is disagreement with that, then we can bikeshed the per-role (permission) and/or per-user trigger (not sure how) in a separate issue. With this patch, any change in mind regarding the trigger boils down to a one-line patch to change the trigger. Unless triggered, there is no performance impact.

Bojhan’s picture

I think a permission is correct, never got why it was taken out.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Reviewed & tested by the community

Just read through the patch and all the changes look fine. Agreed on getting this one quickly since it skews any performance testing unless people specifically know about it and remember to hack core to disable.

catch’s picture

FileSize
17.84 KB

Straight re-roll, no credit please. Was only conflicts in node.api.php so leaving at RTBC.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Sorry for not reviewing sooner, but let's not rush this in... The main reason we have this issue in the first place is that the original patch was rushed in :) -- no one's fault, just the way it happened, but no need to repeat it. If we want something to get in super-quickly to help with performance testing, we could do a quick patch that just added the permission, nothing else.

I think the patch as it is looks pretty good, but I have these questions:

  1. We are moving this line:
    +  drupal_alter('node_build', $build);
    

    from node_build_content() into node_build(). However, there are a number of places in core that call node_build_content() directly. Doesn't that mean they won't receive any alterations to the node content that modules make here?

    We faced the exact same problem in the original issue - see http://drupal.org/files/issues/attached-links-on-everything-473268-158.p... for the way I tried to solve it there (separate $node->content and $node->build_data). I know @sun wasn't happy with the 'build_data' name and this may not be the best way to solve it, but we need to address this issue somehow, I think.

  2. -      $build[$key]['#contextual_links']['block'] = menu_contextual_links('admin/structure/block/manage', array($block->module, $block->delta));
    +      $build[$key]['#contextual_links']['block'] = array('admin/structure/block/manage', array($block->module, $block->delta));
    

    This is starting to look pretty awkward to me. Maybe they should be keyed array elements, something like this?

    $build[$key]['#contextual_links']['block'] = array(
      'path' => 'admin/structure/block/manage',
      'args' => array($block->module, $block->delta),
    );
    

*******

Those were the major issues - the following minor ones should not by themselves hold up a commit.

+    // All contextual links are keyed by the actual "task" path argument,
+    // prefixed with the name of implementing module.

should be "the implementing module"

+ * The comment was built, the module may modify the structured content.

Should be a semicolon rather than a comma.

+    'access contextual links' => array(
+      'title' => t('Access contextual links'),
+      'description' => t('Use administrative links associated with items on a page.'),
+    ),

I know it's been discussed before, but I really don't see why the word "administrative" should be associated with these links - it limits the way they might be used, in my opinion (especially with the current implementation where they are flexible enough that it's easy to add a bunch of them!).

I'm not sure I have a great suggestion though - maybe "Use in-context links for performing actions on items on a page" (needs some massaging)?

+  // Initialize contextual links template variable.

Missing the word "the", I think.

sun’s picture

Initial but a bit wonky stab at issue 1).

Drupal is not at all prepared for build modes.

Moving hook_$object_build_alter() into the corresponding $object_build() function makes totally sense. Without that, you cannot alter the resulting renderable array for an entity.

And we need to do that here, because otherwise, contextual links are not alterable. (but it's not limited to contextual links)

However, there are some wonky-bonky functions in core that totally ignore the rendering system.

This patch simply unsets #theme and #theme_wrappers, but after working on that, I realized that the real solution we need is to assign #theme only for certain build modes, and do not unset $node->content, so those wonky-bonky functions can still just take the built thingy and do whatever they want to do with it. For nodes, this would set #theme for the "teaser" and "full" build modes.

This particular issue is totally unrelated to contextual links, so I'm questioning whether we should move it to a separate issue (and mark this one postponed until done).

catch’s picture

Inconsistencies with building also affects #493314: Add multiple hook for formatters - where we have a field system invocation which only runs on $entity_build_multiple() not $entity_build() right now.

So we do need a separate issue to clean that up, but I also want us to get something in quickly so we can work on performance analysis without having to run patched core for the next month.

Which probably means a patch to just add the permission - which we all agree on, a new issue for build modes, and then postponing the other changes here on the build mode issue?

sun’s picture

Not really. The important change is this:

+++ modules/node/node.module	4 Nov 2009 06:07:15 -0000
@@ -1148,7 +1148,10 @@ function node_build($node, $build_mode =
   // Add contextual links for this node.
-  $build['#contextual_links']['node'] = menu_contextual_links('node', array($node->nid));
+  $build['#contextual_links']['node'] = array('node', array($node->nid));

During building, we only setup hints about the contextual link paths in the form of an array. No functions are invoked, no data is processed, nothing happens, and there is literally zero impact on the rest of the system - until system_preprocess() is invoked for a template, where the access check and dependent processing of the data happens centrally.

I mean, sure, we can squeeze a permission into menu_contextual_links(), but we'll still have many invocations + access checks with that temporary fix then. If you think that this would be sufficient for proper benchmarks now, then I can roll a temporary patch with just those changes.

sun’s picture

So this is what I had in mind and is what I think makes most sense regarding the current state of build modes in core and allows us to properly fix this issue.

moshe weitzman’s picture

if ($build_mode == 'full') {
+    $build['#theme'] = 'comment';
+  }

i'm a little confused about these changes. To me, build modes are a way to give admins control over what appears in render($content) without coding in the theme layer. why would #theme be set for some build modes and not others. how will the other build modules get themed?

$build[$key]['#contextual_links']['block'] = array('admin/structure/block/manage', array($block->module, $block->delta));

so the value of a block is now a regular array, not a render array or a string? am i reading it right? why would we want this? if the block is expensive, we use #pre_render.

should contextual links be exposed as something the admin can enable/disable per build mode?

sun’s picture

how will the other build modules get themed?

I have no idea? node.tpl.php specifically tests for stuff like

  <?php if (!$page): ?>
    <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $node_title; ?></a></h2>
  <?php endif; ?>

i.e. one template is basically suitable for a certain build mode only. In node.tpl.php's case, we always hard-coded two different build modes into the same template, leading to quite some awkward inline logic.

We have no facility to configure the #theme or template to use for a build mode.

Or whether to do not use a template or #theme function at all. That's what those other implementations for book, feed, and search are doing: They want to build a node using a special build mode, and render that structured node array on their own - not invoking any of the default #theme functions nor any of the default template processing.

so the value of a block is now a regular array, not a render array or a string? am i reading it right? why would we want this? if the block is expensive, we use #pre_render.

No, that is only block module's definition for contextual links for a block. $build[$key] is the actual structured block array.

should contextual links be exposed as something the admin can enable/disable per build mode?

That would be logical at least. Likewise, stuff like the author information "Submitted by Foo on 12/34/5678" or node/comment links equally belongs into the build mode. If that would be in place, I would guess that most implementations could even work with the default #theme, because, for example, the book export code only does this to not output that stuff in the export. It would be irrelevant if there would be another DIV around the rendered markup.

I think that this patch lays a solid base for absolutely required further steps in this direction. However, I would suggest to move the remaining of this build mode discussion/fixing into a separate, dedicated issue.

If absolutely required, I can also go back to the approach taken in the previous patch - i.e. unsetting #theme in those other implementations.

yched’s picture

New to this thread, I did not fully grasp how this evolved into such changes around build modes, but sun definitely raises valid questions, which I agree would be best in a separate thread.

- D7 build modes do inherit from D6- where one node template is used for all modes, and it's very possible that we face a roadblock at some point.
But it's true that on it's own, code lke:

   $build += array(
-    '#theme' => 'node',
     '#node' => $node,
     '#build_mode' => $build_mode,
   );
+  // Use the node template for 'teaser' or 'full' build modes.
+  if ($build_mode == 'teaser' || $build_mode == 'full') {
+    $build['#theme'] = 'node';
+  }

Worries me a little.
Is it time for node-<build-mode>.tpl.php template suggestions ? I often end up setting those up in my themes.

- "contextual links + stuff like the author information should be exposed as something the admin can enable/disable per build mode":
That's exactly what I had in mind when I said in a couple places 'the field UI has the potential to become the central hub for most display settings'. It already allows 'pseudo fields' (node components that are not 'fields' - that's for D6 title and body) and lets them display their own 'settings' link. But drag-n-drop reordering doesn't play nice for stuff whose placement is hardcoded in templates (er, like node title currently...)
Also of interest: #616240: Make Field UI screens extensible from contrib - part II

sun’s picture

#499212: Inconsistency in entity view/build/build_alter function invocations and hook names seems to be the proper follow-up issue for this whole build mode thing.

Please also note that the changes applied here are not new - the existing code already does that:

function node_build_content(stdClass $node, $build_mode = 'full') {
...
  // Always display a read more link on teasers because we have no way
  // to know when a teaser view is different than a full view.
  $links = array();
  if ($build_mode == 'teaser') {
    $links['node_readmore'] = array(
      'title' => t('Read more'),
      'href' => 'node/' . $node->nid,
      'attributes' => array('rel' => 'tag', 'title' => strip_tags($node->title[FIELD_LANGUAGE_NONE][0]['value']))
    );
  }
  $node->content['links']['node'] = array(
    '#theme' => 'links',
    '#links' => $links,
    '#attributes' => array('class' => array('links', 'inline')),
  );
...

Thus, I see no reason why the changes in this patch could be rejected. We absolutely need to fix those hard-coded assumptions and usage of build modes, but we can do this after this patch has landed in #499212: Inconsistency in entity view/build/build_alter function invocations and hook names, so we can properly fix the performance issue here and by doing so, also do the first steps in the right direction for #499212.

yched’s picture

The snippet in #44 doesn't shock me: it's node module hardcoding a piece of what 'teaser' means for him.
In #409750: Overhaul and extend node build modes I wrote "The ultimate goal IMO is to have 'modes' be just empty shells, with as little actual behavior hardcoded as possible ".
Sure, it would be best if this was configurable, ideally you would have a 'read_more' pseudo field that you can reorder wherever you want in your node and decide to show or hide in various build modes, adjust the text, etc etc, and I think we'll get there at some point. But the current is not that bad IMO.

This IMO is not comparable to saying 'full and teaser nodes' go through node.tpl.php, for other modes you're on your own.

sun’s picture

well, let's go back to the previous approach then, and proceed from there in the other issues.

moshe weitzman’s picture

Commented out? - //field_attach_prepare_view('node', array($node->nid => $node), $build_mode);

Also, system_preprocess() returns early if user has no access to contextual links?

sun’s picture

+++ modules/node/node.module	5 Nov 2009 17:02:00 -0000
@@ -1190,6 +1193,9 @@ function node_build_content(stdClass $no
   // Build fields content.
+  // @todo field_attach_prepare_view() is only invoked by node_build_multiple(),
+  //   all other entities invoke it _here_.
+  //field_attach_prepare_view('node', array($node->nid => $node), $build_mode);
   $node->content += field_attach_view('node', $node, $build_mode);

well, you need to read the comment above it. I have no idea why it's done differently for nodes (only), and we most likely want to fix that, but not necessarily in this issue/patch.

+++ modules/system/system.module	5 Nov 2009 16:57:47 -0000
@@ -3516,17 +3520,24 @@ function theme_system_settings_form($var
 function system_preprocess(&$variables, $hook) {
...
+  // Initialize the $contextual_links template variable.
+  $variables['contextual_links'] = array();
+
+  // Nothing to do here if the user is not permitted to access contextual links.
+  if (!user_access('access contextual links')) {
+    return;
+  }

Yes, that's the whole purpose of this issue. We want to minimize the performance impact in case no contextual links should be displayed.

Again, we can discuss a different or an additional trigger in #616618: Allow contextual links to be disabled. The only line that may need to be changed then is the condition of this early return.

This review is powered by Dreditor.

sun’s picture

catch mentioned in IRC that the issue with the early return may be that we may want to use that preprocess function for something else someday. Sure. When this sucker has been committed, I want to do a follow-up patch that turns contextual links into an own module anyway. When I did the original implementation, I tinkered for hours to find an approach that would allow us to do that, and only with this patch, it's finally possible to totally disable those links by moving them into an own module.

moshe weitzman’s picture

I did read that comment - it is still rare for us to have commented out php code in CVS. no big deal.

I assumed that system_preprocess did more than just contextual links but it seems not. I usually do a big if() instead of an early return but thats just a matter of taste. moving contextual links to own module sounds good to me.

I'm not too keen on us doing system_build_contextual_links() during preprocess layer. Thats not themeing, IMO. I know that we do this sort of thing elsewhere, but that doesn't make it right. could we use #pre_render for the building and #theme=links such as:


$build['#contextual_links']['node'] = 
  '#pre_render' => array('system_build_contextual_links'),
  '#nid' => $node->nid,
  '#theme => 'links',
)

I know that this patch does not add it, but would it be easy to stop adding stuff to $node that does not belong. specifically, $node->rendered (see book and search modules). I think that's misleading to folks who read he code and expect to always find a rendered presentation inside of $node

sun’s picture

Well, the point is that, with this patch,

  $build['#contextual_links']['node'] = ...

is a meta data structure only. (Note the leading #.) From a design point of view, it follows the principle of

  $build['#attached']['js'] = ...

But having the difference that the data for contextual links is only used for (theme) template rendering. We do not want the contextual links to be rendered within the built content, but instead conditionally process the meta data into a separate template variable that can be positioned + rendered on its own.

We can do further optimizations to the building and rendering process of contextual links, but in this issue, we already crossed the point of talking about a lot of other stuff we badly need to tackle, and instead of making progress, we continue to talk about it.

catch’s picture

Can we either commit this patch as is or agree to commit a patch which only deals with the performance issue and continue this discussion afterwards?

/me goes on profiling strike for now.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Sure - if this gets committed then render improvement will be separate issue.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks sun.

Dries’s picture

I think it would be nice to automatically enable contextual links for users with the 'administrator' role. Should be an install profile thing ...

sun’s picture

We can do that + more after #626286: Make contextual links a module.

sun’s picture

Errr. Actually, no - we have an "administrator" role in core already, which gets all permissions by default. ;)

David_Rothstein’s picture

Status: Fixed » Needs review
FileSize
5.99 KB

As per #32, having these as unkeyed arrays seems a lot different from the standard way we pass this kind of information, and also makes it more difficult for developers to understand...

Otherwise looks good.

sun’s picture

Status: Needs review » Fixed

See #47: We're following a renderable array meta data => function argument conversion pattern we already use elsewhere. So this is consistent with other things.

Anyway, belongs into a new issue.

David_Rothstein’s picture

Status: Fixed » Needs review

This doesn't need a new issue - the syntax was introduced here, and already reviewed well before the patch was committed.

I didn't realize the extent to which '#attached' uses the same style, but the most common way in which that happens, e.g.:

$form['#attached']['library'][] = array('system', 'cookie');

is, in my opinion, much easier to read and understand than this:

$build[$key]['#contextual_links']['block'] = array('admin/structure/block/manage', array($block->module, $block->delta));

Anyway, it's too late to change or improve the '#attached' syntax for Drupal 7. But it shouldn't be too late to improve this one (if there is agreement that it's an improvement).

sun’s picture

Status: Needs review » Needs work
+++ modules/system/system.module	7 Nov 2009 22:49:38 -0000
@@ -3564,12 +3564,19 @@ function system_preprocess(&$variables, 
+ *       'block' => array(
+ *         'parent_path' => 'admin/structure/block/manage',
+ *         'args' => array('system', 'navigation'),

In the actual implementations, we shouldn't use the "parent_" prefix, just 'path' + 'args'.

I'm on crack. Are you, too?

sun’s picture

Also, you need to compare apples with apples:

  $form['scheme'] = array(
    '#type' => 'select',
    '#title' => t('Color set'),
    '#options' => $info['schemes'],
    '#default_value' => $current,
    '#attached' => array(
      // Add Farbtastic color picker.
      'library' => array(
        array('system', 'farbtastic'),
      ),
      // Add custom CSS.
      'css' => array(
        $base . '/color.css' => array('preprocess' => FALSE),
      ),
      // Add custom JavaScript.
      'js' => array(
        $base . '/color.js',
        array(
          'data' => array(
            'color' => array('reference' => color_get_palette($theme, TRUE)),
          ),
          'type' => 'setting',
        ),
      ),
    ),
  );

Yes, we could use named arguments for #attached, but for the simple function argument list specified in #contextual_links, it feels overarching to me.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
4.69 KB

Renamed 'parent_path' to 'path' as per #57. I also split off the code comment fix to #629100: Incorrect PHPDoc in menu_contextual_links() (since that's really a separate bug).

Yeah, I agree that particular #attached example is worse.... I still consider this one an (independent) improvement that we ought to still be able to get in, but if we don't, the world won't end either :)

sun’s picture

Issue tags: -D7 API clean-up

.

Dries’s picture

I think David's patch in #59 is a bit more elegant and easier to read. It is nice to have that extra context.

sun’s picture

Status: Needs review » Postponed

We can do this after #626286: Make contextual links a module landed.

webchick’s picture

Status: Postponed » Active
catch’s picture

sun’s picture

Status: Active » Needs review
FileSize
4.75 KB

Re-rolled against HEAD.

sun’s picture

Title: Decrease performance impact of contextual links » Decrease performance impact of contextual links / change #contextual_links property value
Assigned: sun » Unassigned
Priority: Critical » Normal

This follow-up should really have been in its own issue. :-/

catch’s picture

catch’s picture

Title: Decrease performance impact of contextual links / change #contextual_links property value » Decrease performance impact of contextual links
Priority: Normal » Critical
Status: Needs review » Active
FileSize
90.88 KB

This is still critical, and still slow - currently contextual links takes 12% of the page request when viewing ten nodes. This will only get worse when it's used in the wild. Please open a separate issue for the follow-up to the original patch, marking back to active for the overall performance issue, which seems better tracked here with spin-off patches.

effulgentsia’s picture

What's the current status of this? Any change since #68?

sun’s picture

Component: base system » contextual.module
catch’s picture

Priority: Critical » Normal
Status: Active » Fixed

This isn't going to get fixed without a refactoring, and it's too late for that, so let's just ship with slow experience for admins, which D6 had too.

Status: Fixed » Closed (fixed)

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

catch’s picture

Version: 7.x-dev » 8.x-dev
Priority: Normal » Major
Status: Closed (fixed) » Active

Contextual links are no longer for just admin users.

Wim Leers’s picture

@catch: Is this not also being addressed by the patch at #914382: Contextual links incompatible with render cache?

catch’s picture

@Wim not really. That issue is about cache pollution, this one was for the actual performance implications of generating the links themselves. Contextual links require a router item to point to to check access. This means calling menu_get_item() for each link on a page, often in situations (especially on entities) where the entity is already loaded and has an access method which could just be called directly without having to go via the menu system to find that out.

If contextual link generation moves to a separate request then this won't be happening on initial page generation, so that'd probably make this not 'major' (although it'd be worth looking at just how bad that callback is on a page with lots of contextual links - there's potentially hundreds if they're applied to comments).

Wim Leers’s picture

That makes sense. Thanks for the clarification!

Two things:

  1. I think there were also UX reasons to not have contextual links on comments. (I know I read this somewhere, but can't find it anymore.) If we don't have contextual links on comments, the damage is limited, and possibly (likely?) not worthwhile to solve (not a big enough gain)?
  2. Note that the work in #914382: Contextual links incompatible with render cache opens the door for caching strategies that weren't possible before. E.g. localStorage caching. For the majority of sites, a data-contextual-id|user ID cache key would provide accurate results and would significantly reduce the number of times contextual links have to be rendered.
Wim Leers’s picture

Gábor Hojtsy’s picture

Priority: Major » Normal

If contextual link generation moves to a separate request then this won't be happening on initial page generation, so that'd probably make this not 'major'

.

Wim Leers’s picture

@catch: How is this issue affected by #2084463: Convert contextual links to a plugin system similar to local tasks/actions?

Also: #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash is ready. When that gets committed, each contextual link will only have to be generated once per session! I think that would fully address the concerns here?

Wim Leers’s picture

#2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash was descoped to only get rid of the extra HTTP request per page for Edit module. #2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links now takes care of doing that for Contextual module. Committing that patch would also fix this issue, AFAICT.

dawehner’s picture

Issue summary: View changes
Issue tags: -Contextual links +Needs benchmarks

We certainly need new benchmarks to judge whether in what kind of performance problems we do have.

Wim Leers’s picture

Category: Bug report » Task
Priority: Normal » Minor

#2136507: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for rendering Contextual Links just landed. Each contextual link now only has to be generated once per session!

That doesn't make the generation of each individual contextual link faster though, and that's AFAICT the main concern in this issue.

Taking #75 into account, I think this is now a minor issue. And because of that, it is quite possibly an issue we can simply forget about: it's simply not something costly enough anymore to worry about.

If you disagree, feel free to undo this status/category change.
If you agree, feel free to close this issue :)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Dries committed a8ec4e1 on 8.3.x
    - Patch #607244 by sun: added permission to decrease performance impact...

  • Dries committed a8ec4e1 on 8.3.x
    - Patch #607244 by sun: added permission to decrease performance impact...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Dries committed a8ec4e1 on 8.4.x
    - Patch #607244 by sun: added permission to decrease performance impact...

  • Dries committed a8ec4e1 on 8.4.x
    - Patch #607244 by sun: added permission to decrease performance impact...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • Dries committed a8ec4e1 on 9.1.x
    - Patch #607244 by sun: added permission to decrease performance impact...

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

joseph.olstad’s picture

Status: Active » Fixed

This was resolved and committed years ago.

Wim Leers’s picture

@joseph.olstad++

Status: Fixed » Closed (fixed)

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