Terrible performance when rendering multi-value fields in views

Thanks for posting the profiling info. This is very weird. Even if all the field values are being rendered for each row, I don't see why the extra calls aren't being cached (since views caching is on by default).

\Drupal\views\Entity\Render\EntityFieldRenderer::buildFields

This doesn't seem as efficient as it could be in the case that the same entity spans multiple rows. $entities_by_bundles and $bundle_entities would have the same entity multiple times, and entity view display gets created for the same one each time.

This doesn't seem as efficient as it could be in the case that the same entity spans multiple rows. $entities_by_bundles and $bundle_entities would have the same entity multiple times, and entity view display gets created for the same one each time.

That for itself is a good observation. Rendering fields is incredible hard :) I'm happy for concrete suggestions here :)

On top of that I'm wondering whether we can optimise some of the rendering of date fields. Could \Drupal\Core\Datetime\DateFormatter for example store some static caching of the loaded date formatter?

Two thoughts about \Drupal\views\Entity\Render\EntityFieldRenderer::buildFields before I start on client work

  /**
   * A list of indexes of rows whose fields have already been rendered.
   *
   * @var int[]
   */
  protected $processedRows = [];

PhpStorm says this is not used. Not sure if that matters here.

      $entities_by_bundles = [];
      $field = $this->view->field[current($field_ids)];
      foreach ($values as $result_row) {
        if ($entity = $field->getEntity($result_row)) {
          $entities_by_bundles[$entity->bundle()][$result_row->index] = $this->getEntityTranslation($entity, $result_row);
        }
      }

I think this is the problem. We are building up an array of all of the entities we need, and then looping over then below. In this particular case, the array is filled with the same entity, so it is getting rendered out multiple times.

Not sure if we can fix it here, though. I think that in EntityViewDisplay::buildMultiple() we need to change the loop to see if a particular entity (or group?) has already been rendered, and if so reuse the results instead of just looping and rendering each entry.

?

@bkosborne
Do you mind providing a profiling for the case without rendering a date field, but some other field?

Note that blackfire seems to add some overhead, as it reports that it took the page 4.2 seconds to load. But in my tests in a browser it was ~2.5.

Right, which is fair, as we want the additional information to be collected. It is all about the relative numbers.

70% of the time was spent in views rendering. FieldPluginBase::advancedRender was called 100 times, which called out to field plugin's renderItem method 10,000 times. It seems to be rendering all 100 field deltas for each 100 rows of output.

This is certainly wrong, no question.

Do you mind enhancing the issue summary with your latest results? This helps for new people to know what is going on.

Came across this issue recently on a Multi-value text field with ~100 items. Render time was ~10 seconds for just the one field and it seemed a bit crazy so I dug in.

Was thinking that maybe this could be fixed by just running EntityField::prepareItemsByDelta() in EntityField::getItems() instead of in EntityField::renderItems(). The problem is that by the time the $items makes it to renderItems(), they have already been individually rendered so the damage is done before we pull out the specific items we're actually "rendering". With this change we filter the items by delta before they're actually rendered in advancedRender().

I've attached a patch to show what I'm thinking. I'm sure there's probably some repercussions here but I figure at least this gets the ball rolling!

Go, TestBot, go!

Ah, yes thanks for that! And it passed! :)

I think there are two things to think about here, assuming this is faster (it probably is).

1. Do we have enough test coverage around this?

2. Is this a BC break (esp for contrib or custom modules) by changing what is available in a views hook by pushing something later in the process. This isn't 100% apparent to me; needs some additional debug/tracing.

We have a site that's using a view for slideshow management that went from performing fine to slow enough to cause gateway timeouts in our containerized infrastructure with the 8.7 update. This patch certainly seems to help, but I wonder if some other change to either the rendering or caching systems have exacerbated these performance problems... so far, I haven't found any smoking gun, but this at least mitigates the issue.

patch still applies, is still great!

The questions in #26 have not been answered yet

answering #26

getItems is called before renderItems , and proof is that renderItems takes $items as a parameter

so the nervousness mentioned in #26 is invalid.

"zero" reports of any issues with the patch after 2 years since that the question was posed. We need speed indeed!

Views especially benefits here because we cannot always rely on caching to keep our availability up and our latency low! Performance fixes such as this one actually improve security, lower latency, improves availability, more clients we can serve concurrently.

Win-win here.

Good point Joseph, and besides this has matured very well and still works great. RTBC +1

I think this change makes a lot of sense. I've looked at the code and can't see any reason not to limit the items prior to rendering each individual item. As this is the view rendering pipeline and therefore very complex I'm going to ping view maintainers and ask for a +1 - just in case I'm missing something.

This looks good from a Views rendering pipeline perspective.

We have test coverage for prepareItemsByDelta in:
\Drupal\Tests\views\Kernel\Handler\FieldFieldTest::testComplexRender
\Drupal\Tests\views\Kernel\Handler\FieldFieldTest::testRevisionComplexRender
So we know calling that at a different moment doesn't break anything

And the whole rendering in the rest of \Drupal\Tests\views\Kernel\Handler\FieldFieldTest

So that seems covered.

The daily test is running against 8.5.x though, so we might want to make sure it is still green for 9.1.x. I tried re-queuing it, but when I choose 9.1 it still queues 8.5 ¯\_(ツ)_/¯

As a bug it could be backported

credit to @drclaw for patch 23
re-uploading patch 23 verbatim just to trigger tests again. No change made.

patch applies cleanly to all currently active branches , 8.8.x, 8.9.x, 9.0.x , 9.1.x

***EDIT*** something weird with the testbot, will wait and see what happens once they all run but it looks like I triggered everything on 9.1.x even though I selected other branches.

testbot issues

Re #40 there is something wrong with queuing issues against different branches, it happened to me in another issue. Reported at #3172023: Testing different branches other than the issue branch has stopped working

Committed 074e921 and pushed to 9.1.x. Thanks!

Committing this to 9.1.x only for now - views is complex and it wouldn't be the first time a patch that looks very non-disruptive proved otherwise so going to let this issue sit in 9.1.x for a bit to see if anything breaks. If nothing does we should re-open this for back port to 9.0.x and 8.9.x because the current behaviour to unnecessarily render rows and therby cause timeouts is a bug.