Remove placeholdering of node links [#2556767]

Problem/Motivation

The node links of each node are built using a lazy loader. Which means they are not cached and need to be rebuilt on every page.

On a default article node, that is about 10% of the whole processing time, mostly because of comment.module. Note that this also happens when e.g. displaying a teaser of 20 nodes on the frontpage.

The main reason this was done is that those links are often related to specific permissions and are sometimes not cacheable well. However, when we did this, we didn't have the cache context bubbling and other tools that we have now.

#2530846: Fix and improve comment cache tag usage makes comment links cacheable, the only remaining problem then are statistic counters. See that issue for some discussion on that, still need to take it over to this.

Proposed resolution

Remove the lazy builder, build node links directly. Figure out what to do with statistics view counters.

Remaining tasks

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#12	2556767-node-links-lazybuilder-12-interdiff.txt	1.17 KB	Berdir
#12	2556767-node-links-lazybuilder-12.patch	4.5 KB	Berdir
#12	8.0.x: PHP 5.5 & MySQL 5.5 14,062 pass
#10	2556767-node-links-lazybuilder-8-interdiff.txt	4.04 KB	Berdir
#10	2556767-node-links-lazybuilder-8.patch	4.49 KB	Berdir
#10	8.0.x: PHP 5.5 & MySQL 5.5 13,987 pass
#5	2556767-node-links-lazybuilder-5.patch	470 bytes	Berdir
#5	8.0.x: PHP 5.5 & MySQL 5.5 13,983 pass, 2 fail
#5	node_links_10_nodes_smartcache.png	47.43 KB	Berdir
#5	node_links_10_nodes.png	49.34 KB	Berdir
#5	node_links_single_node.png	45.05 KB	Berdir

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

24 August 2015 at 19:21

Berdir created an issue. See original summary.

Comment #2

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 24 August 2015 at 19:23

See comment #10 in the referenced issue for a start on doing this.

Comment #3

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer commented 24 August 2015 at 20:21

+1 on idea, #10 is API change (makes method non-static)

To properly cache node links each link should carry access somehow, also \Drupal\comment\CommentLinkBuilder::buildCommentedEntityLinks() can be optimized

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 27 August 2015 at 10:54

Remove the lazy builder, build node links directly. Figure out what to do with statistics view counters.

I think you actually mean: remove the '#create_placeholder' => TRUE. Because keeping it as a #lazy_builder means it can be placeholdered if actually necessary.

Comment #5

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 26 September 2015 at 09:20

Status:

Active

» Needs review

File	Size
node_links_single_node.png	45.05 KB
node_links_10_nodes.png	49.34 KB
node_links_10_nodes_smartcache.png	47.43 KB
2556767-node-links-lazybuilder-5.patch	470 bytes
8.0.x: PHP 5.5 & MySQL 5.5 13,983 pass, 2 fail

Oh, that is fancy.

Just removing that line makes the frontpage with a single node and comment (not statistics.module) 9% faster with 10% less function calls. That's almost a critical.

Now, the same with 10 articles. Starting to get critical ;)

And if you're not convinced yet, the same with smartcache.

However, it doesn't fully solve the problem.

a) We don't have auto-placeholdering yet for max-age 0 but that's an easy fix I think?
b) statistics module has no cacheability, also theoretically easy to fix.
c) But we don't really want max-age 0 there (which is the only thing we can do, really), we don't want to make the page 10% slower just to calculate that stupid counter every time. if anything, we would have a placeholder just for statistics so we can just calculate that and avoid the huge comment links overhead. Maybe we could combine lazy-builder with min/max-age caching or something.

Thoughts?

As written before javascript isn't a solution, one random idea I just had is clearing the node cache tag with a certain % chance, e.g. 10% or 1%. but that's, well, random to whether it works.

This will still fail on statistics tests at least. But this might be a big enough difference to just break statistics for a while with a non-critical follow-up to figure that out?

Comment #6

catch

he/him

English

CreditAttribution: catch commented 26 September 2015 at 09:32

Max age is #2559847: Auto-placeholdering for #lazy_builder with bubbling of max-age.

Statistics I would personally let it get stale for a while.

JavaScript might be a solution if we render stale information in PHP then update it in JS?

Comment #7

26 September 2015 at 09:50

Status:

Needs review

» Needs work

The last submitted patch, 5: 2556767-node-links-lazybuilder-5.patch, failed testing.

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 26 September 2015 at 10:04

Issue tags:

+DrupalCon Barcelona 2015

index 37db17d..3f3420c 100644
--- a/core/modules/node/src/NodeViewBuilder.php
+++ b/core/modules/node/src/NodeViewBuilder.php
@@ -41,7 +41,6 @@ public function buildComponents(array &$build, array $entities, array $displays,
             $langcode,
             !empty($entity->in_preview),
           ]],
-          '#create_placeholder' => TRUE,
         );
       }

This being the entire patch, and it speeding things up so significantly by deleting a single line of code … amazing! :D

Berdir <3

Statistics I would personally let it get stale for a while.

Comment #9

26 September 2015 at 10:08

The last submitted patch, 5: 2556767-node-links-lazybuilder-5.patch, failed testing.

Comment #10

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 26 September 2015 at 10:54

Status:

Needs work

» Needs review

File	Size
2556767-node-links-lazybuilder-8.patch	4.49 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,987 pass
2556767-node-links-lazybuilder-8-interdiff.txt	4.04 KB

Ok, what about this?

Added a max age setting to statistics, defaults to 3600. Tests set it to 0 but also test with a non-0 setting.

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 26 September 2015 at 11:06

+++ b/core/modules/statistics/config/schema/statistics.schema.yml
@@ -17,6 +17,9 @@ statistics.settings:
+      label: 'How long that content that shows a views counter is cached'

I think this reads a bit strange. What about How long any statistics may be cached, i.e. the refresh interval.?

+++ b/core/modules/statistics/src/Tests/StatisticsAdminTest.php
@@ -89,6 +92,17 @@ function testStatisticsSettings() {
+    $this->assertText('3 views', 'Views counter was not updated.');
+
   }

Nit: One unnecessary blank line there.

Comment #12

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 26 September 2015 at 11:15

File	Size
2556767-node-links-lazybuilder-12.patch	4.5 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,062 pass
2556767-node-links-lazybuilder-12-interdiff.txt	1.17 KB

Fixed that.

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 26 September 2015 at 11:19

Status:

Needs review

» Reviewed & tested by the community

Comment #14

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 27 September 2015 at 13:30

Status:

Reviewed & tested by the community

» Fixed

berdir++

This patch is a great performance improvement. Committing this under the performance beta eligibility criteria.

Committed d562cd4 and pushed to 8.0.x. Thanks!

Comment #15

27 September 2015 at 13:30

alexpott committed d562cd4 on 8.0.x

Issue #2556767 by Berdir, Wim Leers: Remove placeholdering of node links

Comment #16

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 27 September 2015 at 17:07

So great to see that almost two years after #2151439: Run node op links ("Read more", "Add child page", "Printer-friendly version", "X views" + contrib) through #post_render_cache to prevent render caching granularity being per-user, we are now able to simplify it enormously thanks to #2429257: Bubble cache contexts :)

Marking those as related issues to simplify future archeological work.

Comment #17

joachim CreditAttribution: joachim commented 10 October 2015 at 15:54

> Remove the lazy builder, build node links directly. Figure out what to do with statistics view counters.

Is there any more info about how lazy builders are going to work for individual links? Was there a follow-up for statistics view counters? We need a lazy builder for a single link for Flag module, and I can't see how to do it...

Comment #18

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 11 October 2015 at 09:02

I assume you're talking about a single link as part of a list of links and I don't think that is possible, not easily.

The only thing I could imagine is that you build a link with a manual placeholder in it that is then replaced for both the link and the label. That requires that you cache the access check however, so as I said in #2584647: Flagging access system not extensible, it is then important that deal with AccessResult/cacheability metadata correctly. access logic that shows or doesn't show the link for the node author needs to make sure that the author has a different variant of the cached node output.

Comment #19

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 11 October 2015 at 09:10

That said, I'd suggest you open an issue so that we can try and figure out a way to make that work. I think one of the problems is that a placeholder that isn't actually replaced into a link would then have empty <li> tags around it.