Closed (fixed)
Project:
Drupal core
Version:
7.x-dev
Component:
base system
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
7 Feb 2009 at 22:54 UTC
Updated:
3 Jan 2014 at 00:07 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
catchSlightly better patch - checks if any of the children have a weight at all before doing the uasort.
drupal_render() is called 734 times on my example page.
HEAD:
element_sort()
calls: 5051
self: 192ms
uasort()
calls: 460
self: 106ms
Patch:
element_sort()
calls: 2438
self: 93ms
uasort()
calls: 181
self: 51ms
That's from a total of 1800ms to serve the entire page (single request profiled with xdebug). So we're shaving between 5-8% off the request in this fairly extreme example - which lines up reasonably well with the benchmarks on the first version (which seems not to have attached).
Comment #2
catchAnd a much more aggressive version:
uasort() calls down to 181 and 5ms
element_sort() calls down to 181 and 8ms
ab says 2.11 reqs/second with 90 nodes (around 10% gain on HEAD).
With a somewhat more realistic count of 30 nodes, we get:
HEAD:
4.50 reqs/second
4.45 reqs/second
Patch:
4.93 reqs/second
4.93 reqs/second
Also included additional tests for drupal_render() sorting from #354999: unit tests for drupal_render() with the patch.
Comment #3
catchAfter a discussion with chx, I'm starting to wonder if this new function should just replace element_children() - so we'd have $element_children($elements, $sort = FALSE); It wouldn't make the default case of element_children much more complicated, and I've seen calls to ua_sort() and element_children() outside drupal_render() (not verified that they're always together though). If not though, I think it's worth adding the extra function to remove ~ 5000 function calls on some pages even if it's a bit of duplicate code
Comment #4
catchHere's a version completely swapping out element_children().
Comment #6
webchickcatch and I went around and around on this a bit tonight. While I like the separation between element(_get)_children() and element_sort_children(), the end result is there's a large portion of element_sort_children() which is /exactly/ what element_children() does. It makes more sense to put these together than have code duplication.
Remaining TODOs:
- PHPDoc says @param $elements, function says $element is the name of the param. Should be consistent. Since we're looping over it, probably $elements is the right name.
- There shouldn't be a blank line between the @params and the @return statement.
- Can you please add a newline above
+ // The elements should appear in output in the same order as the array.to satisfy my inner neat-freak? ;)Comment #7
catchThis should cover all those issues.
Comment #8
webchickThis got a nod from Eaton in IRC, and comes with tests which prove it works.
Committed to HEAD. Thanks!
Comment #9
Frando commentedWhy do we sort $children and not $elements? $children is not persistent (won't be passed around with $elements), but $elements is and so is $elements['#sorted']. So if we call element_children in drupal_render and then pass the sorted children keys to drupal_render_children everything is fine, but what happens if #theme is set?
Many #theme functions call drupal_render_children, then. There, $elements['#sorted'] would be TRUE already, but $elements would not actually be sorted, right?
I have not tested this, I just read the patch, so I might be mistaken.
Comment #10
catchFrando, you're right :(
The reason for sorting $children, is because sorting $elements itself means passing a lot of non-children to uasort() in most cases - hitting element_sort() lots and lots of times. An earlier draft of the patch dealt with this by foreaching through the children after they were sorted, unsetting the existing keys and replacing them with the new ones, but didn't make it into the final version. Here's a patch which does that, and also hardens up the test so it covers this bug - I confirmed the new test has a fail unless the element_children() changes are applied too.
Comment #11
bjaspan commentedI was very confused by the fact that D7 seemed to stop supporting #weight until webchick pointed me here... :-)
Comment #12
webchickCommitted to HEAD. Thanks!