Problem/Motivation

This is responsible for 4.5M of memory usage when submitting the modules page.

Views::getApplicableViews() initializes view display plugins.

The Feed display plugin has the following code:

  /**
   * Overrides \Drupal\views\Plugin\views\display\DisplayPluginBase::initDisplay().
   */
  public function initDisplay(ViewExecutable $view, array &$display, array &$options = NULL) {
    parent::initDisplay($view, $display, $options);

    // Set the default row style. Ideally this would be part of the option
    // definition, but in this case it's dependent on the view's base table,
    // which we don't know until init().
    $row_plugins = Views::fetchPluginNames('row', $this->getType(), array($view->storage->get('base_table')));
    $default_row_plugin = key($row_plugins);
    if (empty($this->options['row']['type'])) {
      $this->options['row']['type'] = $default_row_plugin;
    }
  }

This ends up invoking hook_views_data(), with a cold cache, which means 4.5M of memory usage as measured by xhprof.

Proposed resolution

@todo

Remaining tasks

@todo

User interface changes

@todo

API changes

CommentFileSizeAuthor
#9 interdiff.txt1.06 KBdawehner
#9 2495073-9.patch3.21 KBdawehner
#7 interdiff.txt641 bytesdawehner
#7 2495073-7.patch3.21 KBdawehner
#5 interdiff.txt1.66 KBdawehner
#5 2495073-5.patch2.58 KBdawehner
#2 2495073.patch1.19 KBcatch

Comments

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
Issue tags: +VDC

Let's see what breaks if we just rip that out.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2495073.patch1.19 KB
catch’s picture

catch
he/him
Primary language English
commented
Parent issue: » #2494987: [meta-6] Reduce cold cache memory requirements

Status: Needs review » Needs work

The last submitted patch, 2: 2495073.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2495073-5.patch2.58 KB
new interdiff.txt1.66 KB

Some work to get it passing.

Status: Needs review » Needs work

The last submitted patch, 5: 2495073-5.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
Issue tags: +needs profiling
StatusFileSize
new 2495073-7.patch3.21 KB
new interdiff.txt641 bytes

There we go.

I guess it would be great if @catch could verify that this actually improves the memory situation

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/modules/views/src/Plugin/views/display/Feed.php
@@ -150,6 +134,24 @@ protected function defineOptions() {
+    $row_plugins = Views::fetchPluginNames('row', $this->getType(), array($this->view->storage->get('base_table')));
+    $default_row_plugin = key($row_plugins);
+    if (empty($this->options['row']['type']) || $this->options['row']['type'] === 'rss_fields') {

Couldn't this also be moved inside the if() for the 99% case?

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2495073-9.patch3.21 KB
new interdiff.txt1.06 KB

Couldn't this also be moved inside the if() for the 99% case?

Sure

yesct’s picture

yesct commented
Issue tags: +Performance
catch’s picture

catch
he/him
Primary language English
commented

With no xhprof but writing out memory_get_peak_usage() from index.php:

Before:

POST: 74.03 MB
Render: 60.39 MB

After:

POST: 73.62 MB
Render: 64.28 MB

As you can see, there's approx 500kb less memory usage on the submit, but approx 4mb more on the subsequent page render.

As far as I can tell in xhprof, what this represents is the following:

1. In the router rebuild, we don't get the plugin definitions at all any more. However we rebuild the theme registry there, and that does.

2. When building the page, we get the row plugin definitions in the theme registry both times.

What this means is that if we only trigger the route rebuild and not the theme registry rebuild in the same request, then we'll see the improvement. So despite it not helping this scenario, I still think it's a good improvement.

catch’s picture

catch
he/him
Primary language English
commented

While it doesn't improve the numbers here (because we still get row plugins), as a result of trying to unpick this issue I found #2497113: views_theme() gets 19 types of plugin definition, only needs five.

dawehner’s picture

dawehner
Primary language German
commented

So now that this doesn't improve the actual numbers, should we keep the issue open?

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Normal

I think it's good clean-up, but just clean-up at this point.

damiankloip’s picture

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

This looks good to me, good cleanup!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2495073-9.patch, failed testing.

catch queued 9: 2495073-9.patch for re-testing.

catch’s picture

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

catch queued 9: 2495073-9.patch for re-testing.

catch’s picture

catch
he/him
Primary language English
commented

This still improves performance, because views' route subscriber initializes views for the ones that actually have routes still.

catch’s picture

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

Patch came back green.

Committed/pushed to 8.0.x, thanks!

  • catch committed 872b496 on 8.0.x 
    Issue #2495073 by dawehner, catch: Views feed display plugin has to get...

Status: Fixed » Closed (fixed)

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