Problem/Motivation

ElementInfoManager::buildInfo() gets cached plugin definitions and processes all plugins by creating instances, checking for FormElementInterface, adding '#type' to each item, and invoking hook_element_info_alter().

Historically this was considered to be pretty lightweight (?) so running on every page was not the end of the world, as well as allowing themes to alter element_info. However, themes cannot do this anymore as ModuleHandler->alter() is not going to invoke anything in a theme.

Proposed resolution

Cache the processed result and return that from ElementInfoManager::buildInfo(). Keep two layers of caching, plugin definitions, and full altered element info.

From some profiling, doing this processing takes ~20ms - and that's running on every request!

Here is the difference between a cached and uncached run of that element info:

Overall Summary
Total Incl. Wall Time (microsec): 21,439 microsecs
Total Incl. CPU (microsecs): 21,430 microsecs
Total Incl. MemUse (bytes): 2,578,648 bytes
Total Incl. PeakMemUse (bytes): 2,635,856 bytes
Number of Function Calls: 2,224

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Active » Needs review
FileSize
1.93 KB

Here is the patch to add the cache layer in.

damiankloip’s picture

Status: Needs review » Needs work

The last submitted patch, 1: 2368975.patch, failed testing.

Status: Needs work » Needs review

damiankloip queued 1: 2368975.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2368975.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.05 KB
3.42 KB

Let's see how this gets on instead.

Basically, this worked before when it was running on every page:

  /**
   * {@inheritdoc}
   */
  public function getInfo() {
    return array(
      '#method' => 'post',
      '#action' => request_uri(),
      '#theme_wrappers' => array('form'),
    );
  }

Which to me seems like a definite hack :) I have moved it directly into the FormBuilder, we could add a process callback if we wanted to?

Status: Needs review » Needs work

The last submitted patch, 6: 2368975-6.patch, failed testing.

catch’s picture

Priority: Major » Critical
Issue tags: +Performance
Related issues: +#1744302: [meta] Resolve known performance regressions in Drupal 8

Given we have a plugin in core that's making use of the lack of caching, and 46ms looks to be quite a big regression from 7.x, bumping to critical.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.76 KB
1.32 KB

Let's try this. Should fix form unit tests.

Status: Needs review » Needs work

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

dawehner’s picture

Note: We should make it clear in \Drupal\Core\Render\Element\ElementInterface::getInfo()
that this hook is not executed on runtime.

In general I wonder whether we had that kind of caching back in d7? I guess it was less problematic back then, because initializing
hooks is much easier than initializing a plugin.

+++ b/core/lib/Drupal/Core/Render/Element/Form.php
@@ -22,7 +22,7 @@ class Form extends RenderElement {
   public function getInfo() {
...
-      '#action' => request_uri(),
+      '#action' => NULL,

Yeah doing anything dynamic is a bad idea, that is true.

damiankloip’s picture

What would make this clearer would be if it was a static method IMO. Elements is really not a good fit for plugins, but anyway...

martin107’s picture

Status: Needs work » Needs review

Locally I cannot reproduce the fails in FilterHtmlImageSecureTest or ElementInfoManagerTest !!! ( via browser testing ).

So I am about to restest.

Not sure what in head may have changed.

martin107 queued 9: 2368975-9.patch for re-testing.

Status: Needs review » Needs work

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

damiankloip’s picture

Yes, thanks for testings. I think these are probably test env specific issues :/ Looking in to.

martin107’s picture

FWIW - I was using php5.510 - when those tests passed.

dawehner’s picture

What would make this clearer would be if it was a static method IMO. Elements is really not a good fit for plugins, but anyway...

Good point.

damiankloip’s picture

FileSize
4.66 KB
2.43 KB

Let's try this.

damiankloip’s picture

Status: Needs work » Needs review
damiankloip’s picture

Issue summary: View changes

Sorry, initial benchmarks were off a bit I think. It's more like ~20ms. Update IS.

damiankloip’s picture

Issue summary: View changes
jibran’s picture

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -533,6 +533,11 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+      $form['#action'] = $this->requestUri();

+++ b/core/lib/Drupal/Core/Render/Element/Form.php
@@ -22,7 +22,7 @@ class Form extends RenderElement {
+      '#action' => NULL,

TBH I don't like this change. Can we ask form maintainer for the opinion?

catch’s picture

We shouldn't set it dynamically.

Do you mean don't both initializing rather than initializing to NULL? If so might as well leave it unitialized.

damiankloip’s picture

Jibran, you prefer a hack in the form element plugin instead?

The form builder should definitely take control of the action for the form. Not an element plugin :)

jibran’s picture

@damiankloip no. It is in prepareForm it should be ok. Can we remove the NULL initialization? Thanks for the explanation.

damiankloip’s picture

Yeah. I am OK with that, and it seems catch is too.

Rerolling :)

damiankloip’s picture

FileSize
4.64 KB
450 bytes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I think this is perfect

tim.plunkett’s picture

+1

damiankloip’s picture

Great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
@@ -81,22 +88,27 @@ protected function buildInfo() {
   /**
    * {@inheritdoc}
-   *
-   * @return \Drupal\Core\Render\Element\ElementInterface
    */
-  public function createInstance($plugin_id, array $configuration = array()) {
-    return parent::createInstance($plugin_id, $configuration);
+  public function clearCachedDefinitions() {
+    $this->elementInfo = NULL;
+    $this->cacheBackend->delete('element_info_build');
+
+    parent::clearCachedDefinitions();
   }

The removal of the createInstance method is an out-of-scope unrelated change.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.6 KB
679 bytes
jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed b0c49c8 and pushed to 8.0.x. Thanks!

  • alexpott committed b0c49c8 on 8.0.x
    Issue #2368975 by damiankloip: Fixed ElementInfoManager::buildInfo()...

Status: Fixed » Closed (fixed)

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