Problem/Motivation

#3496369: Multiple load path aliases without the preload cache uses Fibers to collect a list of path aliases to load, then loads them when the first fiber in a set is resumed again. This works when multiple placeholders load different path aliases, but not when you have a listing of 50 nodes in a view - because that will be a single placeholder (or not a placeholder at all).

If we render views rows in Fibers, then the path alias preloading should also work for each item in a views listing, which is potentially a reduction of dozens of database queries on cold render caches.

Steps to reproduce

Proposed resolution

Remaining tasks

Blocked on #2511330: Deprecate RendererInterface::render()'s sole $is_root_call parameter and #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3518668

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented

To make this work, should look something like this:

- we need to get the entity ID + langcode in the plugin to pass to the lazy builder, this can be done by slightly reworking/copying getEntityTranslationByRelationship().

- a new service providing the lazy builder callback itself, which takes the entity id + langcode, loads the entity/translation and renders it.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I thought about this, but I'm not quite sure if this is really the right thing to do.

This would pull all these entities out of dynamic page cache as well, is that really what we want to do? Maybe, but also seems to have fairly big implications that we should carefully think through.

should we also exclude them from bigpipe by default?

for views in blocks, this will result in placeholders within placeholders?

what about entity references, comments?

some entities might be not be cacheable, that could be costly. People do use views to render paragraphs for example more often than I'd like.

catch’s picture

catch
he/him
Primary language English
commented

This would pull all these entities out of dynamic page cache as well, is that really what we want to do?

Generally the entity view itself gets render cached, and that will get multiple loaded by CachedPlaceholderStrategy, so it ought to be OK.

some entities might be not be cacheable, that could be costly. People do use views to render paragraphs for example more often than I'd like.

But... not in this case and I hadn't thought about that.

Another way to approach this would to render the entities (or more likely views rows) within fibers but not placeholders.

A bit like #3496835: [PP-2] Render children in fibers although assuming we'll try to avoid doing that actual issue.

catch’s picture

catch
he/him
Primary language English
commented

Looks like StylePluginBase::renderGroupingSets() is the place to try using Fibers, which should produce the same result with path alias preloading but without actual placeholdering.

catch’s picture

catch
he/him
Primary language English
commented
catch’s picture

catch
he/him
Primary language English
commented
catch’s picture

catch
he/him
Primary language English
commented
Title: Views entity rendering should use a lazy builder/placeholder » Use Fibers for rendering views rows
Issue summary: View changes

catch opened merge request !11811

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs work
catch’s picture

catch
he/him
Primary language English
commented

Pushed a branch with a couple of commits, needs cleaning up but it works in combination with #3496369: Multiple load path aliases without the preload cache to preload all the aliases rendered within views rows.

catch’s picture

catch
he/him
Primary language English
commented
Title: Use Fibers for rendering views rows » [PP-2] Use Fibers for rendering views rows
Issue summary: View changes
Status: Needs work » Postponed

The test failures are (mostly? all?) due to known issues - added to issue summary.

catch’s picture

catch
he/him
Primary language English
commented
Title: [PP-2] Use Fibers for rendering views rows » Use Fibers for rendering views rows
Status: Postponed » Needs work

Blockers are in.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I think there's a lot of potential here, it's not uncommon that views display 20, 50 or more results and often, they all contain a media, an alias and so on, so many entities and aliases that could be loaded in bulk.

I had a look at some test fails, but there are things that make this very tricky.

I looked into \Drupal\Tests\views\Kernel\Plugin\RowRenderCacheTest::doTestRenderedOutput(), which uses the counter field, which relies on $this->view->row_index in \Drupal\views\Plugin\views\field\Counter::getValue. this iindex is now incremented at the beginning of \Drupal\views\Plugin\views\style\StylePluginBase::renderFieldRow(), as they all then suspend, it means the counter is then always 6 for every row. The fix for this specific plugin we can switch to $values->index, and I guess deprecate that global counter then.

Going further, it only gets weirder from here. The comment admin test fail has a fatal error because \Drupal\comment\CommentInterface::getCommentedEntity() returns NULL, and also the following warning:

Exception: Warning: Undefined property: Drupal\Core\Field\EntityReferenceFieldItemList::$entity

The thing is, EntityReferenceFieldItemList has a __get() method, but it doesn't go in there, at least not the second time. I can reproduce manually, just create two comments an then visit the comment admin view. I tried to reproduce this in a minimal script, but that works: https://3v4l.org/089uP#v8.4.12

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

This is a known PHP bug: https://github.com/php/php-src/issues/14983, there is a workaround to not use the magic ->get as done in the latest commit here.

mably’s picture

mably commented

[Removed]

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Related issues: +#3565464: Find a way to collect and preload all required entities before rendering

Really nice! Inspired me to create the related issue. I guess both of you @catch and @berdir have some thoughts on it.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

lostcarpark made their first commit to this issue’s fork.

lostcarpark’s picture

lostcarpark commented

Resolved merge conflict caused by #3564937: Remove duplicative caching of views rows, which removed the 'keys' and '#cache_properties' from the $data array. As this has moved into the renderFieldRow() method, automatic rebase wasn't possible.

I expect tests will fail. Not sure if I'm smart enough to resolve them.

lostcarpark’s picture

lostcarpark commented

There were 3 tests failing:

Drupal\Tests\views\Kernel\ViewsTemplateTest::testTemplate

PHP error warning of undefined key "directory" in TwigThemeEngine.php:115. This was caused by non-re-entrant safe code in ThemeManager::getDefaultTemplateVariables, causing the directory key to be referenced before it was set. The fix was to set all array values in a temporary variable first, then assign it to $this->defaultVariables when complete.

Drupal\FunctionalJavascriptTests\Core\Field\TimestampFormatterWithTimeDiffViewsTest::testTimestampFormatterWithTimeDiff

Call to getText() on NULL, resulting in empty markup. Solution was for StylePluginBase to save render state of FieldPluginBase between fibers.

Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryPerformanceTest::testUmamiPerformance

Expected count of queries on "cold" query runs dropped, which hopefully indicates the change is having the desired effect. Needed to adjust expected counts accordingly.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Expected count of queries on "cold" query runs dropped, which hopefully indicates the change is having the desired effect.

Yes this is great.

Haven't reviewed the latest changes yet but this sounds like it's reviewable again, thanks for rebasing and looking at the new test failures.

lostcarpark’s picture

lostcarpark commented

I expect that some additional tests will be required for this change. I'll review tests for other changes to use fibers and see if a similar approach will work here.

lostcarpark’s picture

lostcarpark commented

I removed the $this->view->row_index = $i; lines from core/modules/views/tests/src/Unit/Plugin/field/CounterTest.php, as Counter no longer uses $row_index. I reverted the change to Counter, and verifed that caused CounterTest to fail for 8 of 12 tests.

lostcarpark’s picture

lostcarpark commented

Can we rely on index to always be set in ResultRow? If not, we might need to add a fallback to Counter. If we're confident it will be set, the current logic should be good.