Problem/Motivation

The Attribute classes have slow performance. The architecture is fairly solid and covers the bases but in general due to the constructors it's generally slower than previous drupal_attributes() functions.

To be fair it has more features;) Including the ability to know if attributes have been printed or not.

Anything that can speed this up without removing necessary features would be helpful.

Proposed resolution

  • Provide any improvements to Attribute classes and cleanup
  • Cleanup docs.
  • Add any tests to make sure features aren't removed.

#1757550: [Meta] Convert core theme functions to Twig templates
#2048637: Add #type => 'attributes' and wrap in Attribute class with drupal_pre_render_attributes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

I'm not sure I understand the use case for drupal_pre_render_attributes. Can you explain more clearly? what is a type and what needs to be defined?

joelpittet’s picture

It sounded good to me at the time but I will have to ask Fabianx what he planned to do there again... I understand the Attribute refactor bit better than the element_info stuff. Just added this issue quick as a reminder to him and I to do this.

Fabianx’s picture

@jenlampton: That was what we talked about the performance of Attribute(). By lazy-creating it just when needed, we save a lot! Then we can make them faster!

joelpittet’s picture

Status: Active » Needs review
FileSize
12.24 KB

Ok, so here's pass one at this. The tests I ran seem to pass, let's see what test bot has to say.

I am pretty sure I haven't introduced any regression but I'll have to double check boolean attributes.
Was considering removing empty attributes but alt="" got in the way of that dream.
I did a markup comparison on the homepage and there were no differences, (RDF gave me troubles for a bit again... but I still like RDF:)

Also, some profiling:
Scenario: Bartik, 10 teasers with comment and link row style options in frontpage view.

=== 8.x..8.x compared (51b517a409f93..51b51807d6d9d):

ct  : 106,869|106,869|0|0.0%
wt  : 820,683|821,938|1,255|0.2%
cpu : 764,800|766,123|1,323|0.2%
mu  : 39,952,280|39,952,280|0|0.0%
pmu : 40,119,728|40,119,728|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51b517a409f93&...

=== 8.x..2006282-attribute-refactor compared (51b517a409f93..51b518680585e):

ct  : 106,869|105,753|-1,116|-1.0%
wt  : 820,683|815,480|-5,203|-0.6%
cpu : 764,800|761,824|-2,976|-0.4%
mu  : 39,952,280|39,919,200|-33,080|-0.1%
pmu : 40,119,728|40,087,256|-32,472|-0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51b517a409f93&...

I think I can make this faster yet.

This tackles the first part of the problem, less classes (3 less in this case), I tried making it 1 Attribute class but since all Attribute's needed to know when they were printed in the cases when they were pulled out that posed some problems.

Status: Needs review » Needs work

The last submitted patch, 2006282-attribute-refactor.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

Boolean attributes are not taken into account in #4

In terms of preference wise I would like to see the input values remain intact until printed, possible not modified at all until printed. Boolean attributes have a special case where their name can't exist, not just the value. @see http://www.whatwg.org/specs/web-apps/current-work/multipage/common-micro...

This can be easily taken care of when printing all the attributes, but in the case where that attribute is being taken out eg: {{ attributes.disabled }} the only reasonable case would be to leave it as a boolean so it could be setup like this: {% if attributes.disabled %} disabled{% endif %} or maybe {{ attributes.disabled ? 'disabled' : '' }}

Attached is just a couple of tests added to make sure Boolean attributes are tested.

Status: Needs review » Needs work

The last submitted patch, 2006282-boolean-attribute-tests.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.05 KB

Ok well I reverted thing back to the way they were and did minor touch-ups instead to help with performance and docs and the test above in #6 was made correctly.

Upload of the profiling was stalling so here are the local results. Biggest win was the removal of the array_map()

=== 8.x..8.x compared (51b53cab7ba60..51b53d37300cc):
ct  : 106,869|106,869|0|0.0%
wt  : 829,323|827,697|-1,626|-0.2%
cpu : 766,732|768,771|2,039|0.3%
mu  : 39,952,280|39,952,280|0|0.0%
pmu : 40,119,728|40,119,728|0|0.0%

=== 8.x..2006282-attribute-refactor compared (51b53cab7ba60..51b53d9643dd8):

ct  : 106,869|106,434|-435|-0.4%
wt  : 829,323|819,509|-9,814|-1.2%
cpu : 766,732|764,847|-1,885|-0.2%
mu  : 39,952,280|39,952,424|144|0.0%
pmu : 40,119,728|40,119,936|208|0.0%
joelpittet’s picture

Removed my debug messages, and added a new test and fix for name being check_plained.

First should fail, second should pass.

Status: Needs review » Needs work

The last submitted patch, 2006282-10-attribute-refactor.patch, failed testing.

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs work » Needs review
FileSize
6.73 KB

Hmm, forgot my fix in #8

Status: Needs review » Needs work

The last submitted patch, 2006282-12-attribute-refactor.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
joelpittet’s picture

@Fabianx could you provide a bit more on your idea of getting #attributes in the #type elements?

joelpittet’s picture

Issue summary: View changes

Change direction, refactoring the attributes class was too hard.

joelpittet’s picture

Title: Refactor Attribute classes and add drupal_pre_render_attributes » Refactor Attribute classes for performance.
Assigned: joelpittet » Unassigned
Issue tags: +Performance

Split the issues up. This bit should just go in.

Fabianx’s picture

joelpittet’s picture

Issue tags: -Performance

Status: Needs review » Needs work
Issue tags: +Performance

The last submitted patch, 2006282-12-attribute-refactor.patch, failed testing.

markhalliwell’s picture

Issue tags: +Needs reroll

.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.51 KB
6.66 KB

Here's a reroll but the tests were moved and changed so I don't know if I did the change right and they were not acting normal locally... so maybe someone can have a look at that?

Status: Needs review » Needs work

The last submitted patch, 2006282-20-attribute-refactor.patch, failed testing.

joelpittet’s picture

+++ b/core/tests/Drupal/Tests/Core/Common/AttributesTest.php
@@ -31,9 +31,13 @@ public static function getInfo() {
+      array(array('&"\'<>' => 'value'), ' &amp;&quot;&#039;&lt;&gt;="value"', 'HTML encode attribute names.'),

FYI, it's failing on this test I added for the label encoding check I added.

markhalliwell’s picture

+++ b/core/tests/Drupal/Tests/Core/Common/AttributesTest.php
@@ -31,9 +31,13 @@ public static function getInfo() {
+      array(array('&"\'<>' => 'value'), ' &amp;&quot;&#039;&lt;&gt;="value"', 'HTML encode attribute names.'),
       array(array('title' => '&"\'<>'), ' title="&amp;&quot;&#039;&lt;&gt;"', 'HTML encode attribute values.'),

Should the "value" be the key?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
7.09 KB

Ok I fixed it, seemed there were other places that needed conversion from check_plain to String::checkPlain

@Mark Carver nope, one is checking that the value is escaped, the other is checking that the attribute's name is escaped. Also, it worked before just the loaded test syntax was throwing me and the output in the tests is not so pretty as I'd expect. ie: ->testDrupalAttributes with data set #0() vs HTML encode attribute names.

joelpittet’s picture

Tagging and adding tests only to show that #24 fails without the refactor and cleanup. Expect this patch to fail:)

Hoping to get this in quick because:

  • it gives a 1% wall time preformance gain accross the board
  • fixes some attribute label security
  • cleans up some documentation

Status: Needs review » Needs work

The last submitted patch, 2006282-25-attribute-refactor-tests-only.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Exactly as planned, please review #24 thanks:)

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -64,6 +63,18 @@ public function offsetGet($name) {
+    $this->storage[$name] = $this->createAttributeValue($name, $value);
...
+ */
+  protected function createAttributeValue($name, $value) {
     if (is_array($value)) {

What do we gain by using a helper function here?

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -73,13 +84,7 @@ public function offsetSet($name, $value) {
-    // The $name could be NULL.
-    if (isset($name)) {
-      $this->storage[$name] = $value;
-    }
-    else {
-      $this->storage[] = $value;
-    }

Why is this check no longer needed?

What happens if $name is NULL now?

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -103,9 +108,9 @@ public function __toString() {
-        $rendered = is_object($value) ? $value->render() : (check_plain($name) . ' = "' . check_plain($value) . '"');
+        $rendered = $value->render();

Good catch! If the above test works, this is_object is unnecessary.

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -117,9 +122,7 @@ public function __toString() {
-      if (is_object($value)) {
-        $this->storage[$name] = clone $value;
-      }
+      $this->storage[$name] = clone $value;

Same here, good catch!

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -129,12 +132,4 @@ public function  __clone() {
-  /**
-   * Returns the whole array.
-   */
-  public function value() {
-    return $this->value;

Why is this no longer needed / wanted?

+++ b/core/lib/Drupal/Core/Template/AttributeArray.phpundefined
@@ -15,13 +15,13 @@
- *  $attributes = new Attribute(array());
+ *  $attributes = new Attribute();

Do we change that or was that always already the case?

+++ b/core/lib/Drupal/Core/Template/AttributeArray.phpundefined
@@ -67,7 +67,7 @@ public function offsetExists($offset) {
-    return implode(' ', array_map(array('Drupal\Component\Utility\String', 'checkPlain'), $this->value));
+    return String::checkPlain(implode(' ', $this->value));

A very nice catch!

+++ b/core/lib/Drupal/Core/Template/AttributeValueBase.phpundefined
@@ -53,7 +55,7 @@ public function __construct($name, $value) {
-    return $this->name . '="' . $this . '"';
+    return String::checkPlain($this->name) . '="' . $this . '"';

Yes, indeed a missing check_plain.

Nice new tests, but I have several open questions that need answering before I could RTBC this. Thanks!

joelpittet’s picture

Thanks for the review @Fabianx! Sorry for the late reply.

The added createAttributeValue() function was added for code clarity. offsetSet() was doing the 'setting' and 'constructing' of the values it was setting. The separate function is to separate those two actions.

// The $name could be NULL.
When would $name be NULL? If this is a use-case maybe it should have a test too?

The removal of that function "Returns the whole array." never gets used, but I guess someone could use it... I'll add it back.

That removal of the array() inside the Attribute() constructor was already the case in that example.

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs work » Needs review
FileSize
6.49 KB

Hmm seems someone else removed the value() call already... not where when that happened, but here's a re-roll nevertheless.

Status: Needs review » Needs work

The last submitted patch, 2006282-30-attribute-refactor.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
612 bytes
6.3 KB

Ah looks like someone put that checkPlain String reference in already.

Status: Needs review » Needs work
Issue tags: -Performance, -Quick fix, -Security

The last submitted patch, 2006282-32-attribute-refactor.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: +Performance, +Quick fix, +Security
joelpittet’s picture

tagging

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -104,9 +110,9 @@ public function __toString() {
    -          $return .= " $rendered";
    +          $return .= ' ' . $rendered;
    

    Isn't the first code a little bit faster?

  2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -118,9 +124,7 @@ public function __toString() {
       public function  __clone() {
         foreach ($this->storage as $name => $value) {
    -      if (is_object($value)) {
    -        $this->storage[$name] = clone $value;
    -      }
    +      $this->storage[$name] = clone $value;
    ...
       }
    

    Do we really still have to override the clone method given that storage is the only variable on the object?

dawehner’s picture

joelpittet’s picture

@dawehner From the searching around I've done the single quote concatenation is very marginally faster. Just due to PHP having to parse the string first for variables first then replace them.

Re the clone override: I could be wrong but I believe that it's doing that to avoid any references inside the array when copying the Attribute. Each item in $storage is an object of AttributeValueBase and that's why i've removed the is_object() check.

Removing the preformance because going through with @msonnabaum it wasn't a huge performance benefit as I originally thought. Mostly cleanup/security/dx.
https://gist.github.com/msonnabaum/ff0b357b3cd0381b3b47

joelpittet’s picture

Title: Refactor Attribute classes for performance. » Refactor Attribute classes - Cleanup, Security, and Readability and minor performance

Re-title

star-szr’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -65,6 +65,18 @@ public function offsetGet($name) {
    +/**
    + * Creates the different types of attribute values.
    + * @param string $name
    + *   The attribute name.
    + * @param mixed $value
    + *   The attribute value.
    + * @return object
    + */
    

    Add a blank line after the summary line and a blank line above the @return line please.

    Oh and I think the @return needs to be documented as to what is being returned per http://drupal.org/node/1354#return.

  2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -74,13 +86,7 @@ public function offsetSet($name, $value) {
    -    // The $name could be NULL.
    -    if (isset($name)) {
    -      $this->storage[$name] = $value;
    -    }
    -    else {
    -      $this->storage[] = $value;
    -    }
    

    I think this is fine, I see this check was added in #1290694-15: Provide consistency for attributes and classes arrays provided by template_preprocess() but not much justification there and it was pretty early in the stage of that patch.

  3. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -104,9 +110,9 @@ public function __toString() {
    -        $rendered = is_object($value) ? $value->render() : (String::checkPlain($name) . ' = "' . String::checkPlain($value) . '"');
    +        $rendered = $value->render();
    

    Nice - the old fallback to print the string had spaces around the = sign as well!

  4. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -118,9 +124,7 @@ public function __toString() {
       public function  __clone() {
         foreach ($this->storage as $name => $value) {
    ...
    +      $this->storage[$name] = clone $value;
         }
       }
    

    I walked through this with @joelpittet and this makes sense to keep the clone. There is some discussion around #1290694-55: Provide consistency for attributes and classes arrays provided by template_preprocess() about this and I'm convinced the magic clone method is needed.

Also, moving value() to storage() happened here: #2083941: \Drupal\Core\Theme\Attribute->value() is named wrong and does not work

So other than the very minor docs from the first point I think this is ready to go.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Needs review
FileSize
6.43 KB
1.13 KB

Thanks @Cottser, I've added docs for 1 from #40

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me! If it's not green testbot will kick it back but I think that would just be a random failure.

  • Cleans up example code in @code…@endcode to match with actual usage.
  • Good optimizations, fixes, and simplifications throughout.
  • Added test coverage.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Security, -clean-up

The last submitted patch, 2006282-41-attribute-refactor.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Security, +clean-up
star-szr’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Nice work.

Committed 682adb0 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

cleanup