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.
Related Issues
#1757550: [Meta] Convert core theme functions to Twig templates
#2048637: Add #type => 'attributes' and wrap in Attribute class with drupal_pre_render_attributes
Comment | File | Size | Author |
---|---|---|---|
#41 | interdiff.txt | 1.13 KB | joelpittet |
#41 | 2006282-41-attribute-refactor.patch | 6.43 KB | joelpittet |
#32 | 2006282-32-attribute-refactor.patch | 6.3 KB | joelpittet |
#32 | interdiff.txt | 612 bytes | joelpittet |
#30 | 2006282-30-attribute-refactor.patch | 6.49 KB | joelpittet |
Comments
Comment #1
jenlamptonI'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?Comment #2
joelpittetIt 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.
Comment #3
Fabianx CreditAttribution: Fabianx commented@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!
Comment #4
joelpittetOk, 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.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51b517a409f93&...
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.
Comment #6
joelpittetBoolean 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.
Comment #8
joelpittetOk 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()
Comment #9
joelpittetRemoved my debug messages, and added a new test and fix for name being check_plained.
First should fail, second should pass.
Comment #11
joelpittetHmm, forgot my fix in #8
Comment #13
star-szr#11: 2006282-12-attribute-refactor.patch queued for re-testing.
Comment #14
joelpittet@Fabianx could you provide a bit more on your idea of getting #attributes in the #type elements?
Comment #14.0
joelpittetChange direction, refactoring the attributes class was too hard.
Comment #15
joelpittetSplit the issues up. This bit should just go in.
Comment #16
Fabianx CreditAttribution: Fabianx commentedI agree, lets get this in first then do #2048637: Add #type => 'attributes' and wrap in Attribute class with drupal_pre_render_attributes. Thanks!
Comment #17
joelpittet#11: 2006282-12-attribute-refactor.patch queued for re-testing.
Comment #19
markhalliwell.
Comment #20
joelpittetHere'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?
Comment #22
joelpittetFYI, it's failing on this test I added for the label encoding check I added.
Comment #23
markhalliwellShould the "value" be the key?
Comment #24
joelpittetOk 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()
vsHTML encode attribute names.
Comment #25
joelpittetTagging 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:
Comment #27
joelpittetExactly as planned, please review #24 thanks:)
Comment #28
Fabianx CreditAttribution: Fabianx commentedWhat do we gain by using a helper function here?
Why is this check no longer needed?
What happens if $name is NULL now?
Good catch! If the above test works, this is_object is unnecessary.
Same here, good catch!
Why is this no longer needed / wanted?
Do we change that or was that always already the case?
A very nice catch!
Yes, indeed a missing check_plain.
Nice new tests, but I have several open questions that need answering before I could RTBC this. Thanks!
Comment #29
joelpittetThanks 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.
Comment #30
joelpittetHmm seems someone else removed the value() call already... not where when that happened, but here's a re-roll nevertheless.
Comment #32
joelpittetAh looks like someone put that checkPlain String reference in already.
Comment #34
joelpittet#32: 2006282-32-attribute-refactor.patch queued for re-testing.
Comment #35
joelpittettagging
Comment #35.0
joelpittetUpdated issue summary.
Comment #36
dawehnerIsn't the first code a little bit faster?
Do we really still have to override the clone method given that storage is the only variable on the object?
Comment #37
dawehner.
Comment #38
joelpittet@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
Comment #39
joelpittetRe-title
Comment #40
star-szrAdd 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.
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.
Nice - the old fallback to print the string had spaces around the = sign as well!
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.
Comment #41
joelpittetThanks @Cottser, I've added docs for 1 from #40
Comment #42
star-szrLooks good to me! If it's not green testbot will kick it back but I think that would just be a random failure.
Comment #44
star-szr#41: 2006282-41-attribute-refactor.patch queued for re-testing.
Comment #45
star-szrComment #46
alexpottNice work.
Committed 682adb0 and pushed to 8.x. Thanks!
Comment #47.0
(not verified) CreditAttribution: commentedcleanup