Updated: Comment #65

Problem/Motivation

Drupal 8 will ship with far more JavaScript. Drupal 8 will need to perform well on mobile.

This means we must allow JavaScript code to apply smart client-side caching strategies in order to provide a fast experience. Two of the most obvious things to cache are: 1) user-specific data, 2) permission-dependent data.

Currently, Drupal 8 performs 2 additional HTTP requests per page load that hit Drupal:

  • 1 HTTP request/page load for rendering contextual links
  • 1 HTTP request/page load for retrieving in-place editing metadata

(Assuming the user has permission to both use contextual links and in-place editing, of course.)

Both of these HTTP requests are necessary because the necessary metadata cannot be embedded in the page itself, because that would break render caching.

However, they result in:

  1. worse front-end performance
  2. more networking activity (hence reduced battery life)
  3. worse site scalability (because they each require Drupal to bootstrap)

#2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash gets rid of the 1 HTTP request/page load for retrieving in-place editing metadata. This issue gets rid of the one for rendering contextual links.

Proposed resolution

Use the same approach as the one in #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash: depend on drupalSettings.user.permissionsHash.

However, more care is needed in this case, since contextual links for entities may vary depending on the state of the entity. So, the contextual links may only be cached for as long as the entity remains unchanged (identical EntityChangedInterface::getChangedTime()).

Remaining tasks

Handle entity changes.

User interface changes

None.

API changes

  • Internals change: contextual links are rendered without the ?destination=<current_page> parameter, this is added in JS.
CommentFileSizeAuthor
#25 client_side_caching_contextual-2136507-25.patch17.26 KBwim leers
#17 interdiff.txt758 byteswim leers
#17 client_side_caching_contextual-2136507-17.patch17.44 KBwim leers
#11 interdiff.txt1.17 KBwim leers
#11 client_side_caching_contextual-2136507-11.patch17.54 KBwim leers
#9 client_side_caching_contextual-2136507-9.patch16.89 KBwim leers
#5 interdiff.txt1.17 KBwim leers
#5 client_side_caching_contextual-2136507-5.patch31.79 KBwim leers
#3 interdiff.txt2.66 KBwim leers
#3 client_side_caching_contextual-2136507-3.patch16.89 KBwim leers
#2 interdiff.txt2.31 KBwim leers
#2 client_side_caching_contextual-2136507-2.patch13.18 KBwim leers
#1 client_side_caching_contextual-2136507-1.patch11.46 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Postponed
StatusFileSize
new client_side_caching_contextual-2136507-1.patch11.46 KB

Blocked on #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash (to provide drupalSettings.user.permissionsHash). Initial patch spliced from that issue's patch.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new client_side_caching_contextual-2136507-2.patch13.18 KB
new interdiff.txt2.31 KB

However, more care is needed in this case, since contextual links for entities may vary depending on the state of the entity. So, the contextual links may only be cached for as long as the entity remains unchanged (identical EntityChangedInterface::getChangedTime()).

I think that conceptually, it makes a ton of sense to rely on EntityChangedInterface::getChangedTime(). That still leaves two problems:

  1. Assuming we know with which entity a contextual link is associated, how do we send the last change time to the client? Do we use a data-contextual-entity-last-changed attribute, or something else?
  2. How do we know with which entity a contextual link is associated? The contextual links API currently does not provide a way to provide this metadata.

The only way I see we can do this without introducing API changes is to make every entity type that adds contextual links to its entities, also set #contextual_links's 'metadata' key to array('changed' => $entity->getChangedTime()). That immediately resolves the problem.
The only downside: the client-side cache entry will continue to exist indefinitely, since an updated entity results in an updated cache key. However, since sessionStorage is used, it will only continue to live for the duration of the tab remaining open, so there's only a minor cost.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed » Needs review
StatusFileSize
new client_side_caching_contextual-2136507-3.patch16.89 KB
new interdiff.txt2.66 KB

Reroll against HEAD. Most things remained the same. But with #2141055: When multiple instances of the same entity on one page, only the first can be edited and #2075185: When an entity is in-place edited (i.e. saved), other instances of that entity on the same page are not updated (no propagation) having landed, I had to improve a few things in Edit's JS to prevent race conditions while using the client-side cache.

The last submitted patch, 3: client_side_caching_contextual-2136507-3.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new client_side_caching_contextual-2136507-5.patch31.79 KB
new interdiff.txt1.17 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

5: client_side_caching_contextual-2136507-5.patch queued for re-testing.

The last submitted patch, 5: client_side_caching_contextual-2136507-5.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Lazy testbot.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new client_side_caching_contextual-2136507-9.patch16.89 KB

Straight reroll.

Status: Needs review » Needs work

The last submitted patch, 9: client_side_caching_contextual-2136507-9.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new client_side_caching_contextual-2136507-11.patch17.54 KB
new interdiff.txt1.17 KB
moshe weitzman’s picture

moshe weitzman commented 
+++ b/core/modules/contextual/js/contextual.js
@@ -17,24 +17,51 @@ var options = $.extend(drupalSettings.contextual,
+    _.chain(storage).keys().each(function (key) {

Where could I learn what _.chain(storage) means? I get that it refers to client side storage api but the syntax is unfamiliar to me.

This issue could use some better js review than me.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#12: that's underscore.js. The "chain" just means "let me call multiple collection methods in a row, i.e. don't yet return a hash". That's why I can call keys() followed immediately by each(). i.e. these are equivalent:

_.chain(storage).keys().each(function (key) { … });

and

var keys = _.keys(storage);
_.each(keys, function (key) { … } );
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#607244: Decrease performance impact of contextual links

AFAICT this would also solve #607244: Decrease performance impact of contextual links.

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.php
@@ -285,7 +285,7 @@ public function testBlockContextualLinks() {
-    $this->assertIdentical($json[$id], '<ul class="contextual-links"><li class="block-configure odd first"><a href="' . base_path() . 'admin/structure/block/manage/' . $block->id() . '?destination=test-page">Configure block</a></li><li class="views-uiedit even last"><a href="' . base_path() . 'admin/structure/views/view/test_view_block/edit/block_1?destination=test-page">Edit view</a></li></ul>');
+    $this->assertIdentical($json[$id], '<ul class="contextual-links"><li class="block-configure odd first"><a href="' . base_path() . 'admin/structure/block/manage/' . $block->id() . '">Configure block</a></li><li class="views-uiedit even last"><a href="' . base_path() . 'admin/structure/views/view/test_view_block/edit/block_1">Edit view</a></li></ul>');

+++ b/core/modules/contextual/contextual.module
@@ -268,9 +268,6 @@ function contextual_pre_render_links($element) {
-    $item['localized_options'] += array('query' => array());
-    $item['localized_options']['query'] += drupal_get_destination();
-    $links[$class] += $item['localized_options'];

    Isn't it a nice feature to get redirected after changing a block? 

  2. +++ b/core/modules/contextual/js/contextual.js
@@ -17,24 +17,51 @@ var options = $.extend(drupalSettings.contextual,
+var cachedPermissionsHash = storage.getItem('Drupal.contextual.permissionsHash');
+var permissionsHash = drupalSettings.user.permissionsHash;

    Yes, permissions could work, though routes can have arbitrary access checking 

  3. +++ b/core/modules/edit/js/edit.js
@@ -235,6 +231,29 @@ function initEdit (bodyElement) {
+function processEntity (entityElement) {
...
+}
...
+ * @return Number
+ *   The entity instance ID that was assigned.

    I don't see a return statement in that function.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

1. Destination is added client-side now. See function initContextual ($contextual, html).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new client_side_caching_contextual-2136507-17.patch17.44 KB
new interdiff.txt758 bytes

#15.1: answered by #16 — thanks penyaskito :)

#15.2: I know, I agree. That was also catch's concern in #2005644. The solution I proposed here is to cache on the client side based on TWO things: permissions AND the last changed time of the entity. As I said in the issue summary:

However, more care is needed in this case, since contextual links for entities may vary depending on the state of the entity. So, the contextual links may only be cached for as long as the entity remains unchanged (identical EntityChangedInterface::getChangedTime()).

That still won't help if the arbitrary access checks depend on external factors like temperature, time of day, or whatnot, but it will be sufficient to handle e.g. workflow status.

#15.3: good catch, thanks!

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

OK, thanks for the js education and for the better doxygen. Ready to fly.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: wim leers » catch

This patch looks catch-iferous!

dawehner’s picture

dawehner
Primary language German
commented

Oh thank you ...

That still won't help if the arbitrary access checks depend on external factors like temperature, time of day, or whatnot, but it will be sufficient to handle e.g. workflow status.

Well, doesn't that basically mean we introduce a new problem here? Given that this issue is major this could be okay for sure, but it would be at least good to know whether we have a long term plan to allow people in contrib to swap things out if needed.

catch’s picture

catch
he/him
Primary language English
commented

We have #2099137: Entity/field access and node grants not taken into account with core cache contexts open - not for client side caching, but via that issue it will be possible to add something like a time-of-day granularity to entity render cache IDs. I'd expect to then be able to do the same for the client side cache ID. It might not be very clean, but I don't think we're preventing the temperature access module from being written, just making it a bit more complex (which is fine IMO - not many sites/modules need time of day/temperature based access).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Like catch says, you can do the same for client-side cache IDs: just add more metadata than that what's already being set.
Furthermore: it's cached client-side in sessionStorage, not localStorage. That means it's cached on a per-browser tab basis. In other words, just like PHP doesn't allow you to mess things up too badly, neither will this client-side caching.

catch’s picture

catch
he/him
Primary language English
commented

17: client_side_caching_contextual-2136507-17.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: client_side_caching_contextual-2136507-17.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new client_side_caching_contextual-2136507-25.patch17.26 KB

Straight reroll. This was broken by #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -sprint

Awesome, thank you! :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: catch » wim leers

Status: Fixed » Closed (fixed)

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