Performance: Do not sort already rendered elements in drupal_render()

Oh my. Good catch, catch. This looks like a valid bug fix to me. (2882 is still slow though.)

Better patch, this one saves 88 calls to drupal_render() itself, uasort numbers are the same.

Same benchmarks as http://ca.tchpole.net/node/7 for 90 nodes.

HEAD 1.82 reqs/sec
Patch: 1.98 reqs/sec

Benchmarked the uasort patch and it gets us about 1.92/sec - so #2 is definitely the better one so far.

Those numbers above are number of function calls rather than time spent in the function. Here's screenshots of kcachegrind sorted by SELF (the cachegrind files themselves are 5mb each, but I can make them available if necessary).

Moshe asked for benchmarks with APC enabled.

HEAD:
2.48 reqs/sec

Patch:
2.69 reqs/sec

Please ignore the clunkiness, easy to tidy up, but someone please benchmark 30 nodes on a front page with this patch. I get 2.71 reqs/second versus 1.82 in HEAD on my nasty 90 nodes page. This cuts the calls to drupal_render() down to 362, from 630.

Thats an 8.5% speedup with APC. I can't believe that number, but even a quarter of that is nice.

I don't understand the theme.inc change. Why can't drupal_render() keep track of what it has rendered and sorted already?

Also, we really need to add #sorted anyway. In #351235: hook_page_alter(), we are pulling terms, nodes, blocks out of the DB in order and then adding them to a render array with #weight which will do another sort. I'd love to add them with #sorted and be done with it.

moshe - that wasn't a real patch, it looked like #rendered wasn't doing as much as it should, so was trying to verify it. But looks like my benchmarks were wrong in that case.

We already check for #sorted in drupal_render(), so adding #sorted = TRUE inside drupal_render() shouldn't be necessary if we're adding #rendered further up - we should never get there twice in a request any more. However we should definitely be adding #sorted to pre-sorted elements though, I've added that for $terms here.

With 30 nodes this time, on taxonomy/term/n:
HEAD:
5.77 reqs/sec

Patch, with the#rendered change and #sorted = TRUE on $terms:
6.45 reqs/sec

drupal_render() itself is down from 210 to 182 (Self. time 21718 vs. 11323 - a lot of these are early returns now it seems)
uasort is down from 210 to 92 (Self. time 18296 vs. 3039)
cachegrind says the calls to element_sort are down from 962 to 168 (Self. time 32472 vs 5229)
element_children() down from 208 to 122 (Self. time 13603 vs. 4505)

Total time as recorded by cachegrind:
HEAD:
731463
Patch:
599155

Attached the patch and before/after kcachegrind screenshots, in the second element_sort() actually gets knocked off the first page of slow functions.

I'm sure we can squeeze some more out of here, but this is probably good as a first pass, so marking back to CNR.

Did some profiling against HEAD a month ago and found a couple more optimisations.

Added a $sort parameter to drupal_render() so we can use that rather than cluttering up pre-process functions with lots of if statements and setting #sorted directly. Also we now only run uasort() on $elements which have children - we can't sort something that has no children, but we were previously trying to do this on $node->body, in which everything is a '#property', so wasting a lot of time doing a no-op sort.

This means that with 30 nodes, the number of calls to uasort is 30 - same as it used to be before the recent node rendering patches landed, down from 210 in HEAD.

Very nice patch. A couple nits:

- I do think we should sort $node->content['links'] like we sort $node->content later. It is a pain to require that modules insert themselves where they belong. That brings back all the badness of system.weight

- The remaining sort preventions in theme.inc are for terms and comments. I agree that those are desireable, but they should be done at time of adding to $node->content. It is comment module who knows that its data has been sorted; so comment.module should add #sorted = TRUE during comment_nodeapi_view() (for example).

1. OK, but we should probably also add a '#weight' property to the documentation for hook_link() as well then?
2. Agreed.

Patch has these changes

Ahem - there is no hook_link() anymore for nodes. Adding links is just like adding stuff to the body. The xample code for hook_nodeapi_view() in the APi docs does have #weight property.

I'm satisfied with this patch now. Could use a look over from chx or eaton.

I created 500 nodes, 3 comments each, using devel_generate. Also configured the "Add new comment" form to be displayed on the node page. Running ab on the front page (/node) and one of the nodes gives me the following numbers:

ab -n 50 -c 2 ...

and

I.e. no difference on a single node, but nice improvement for the front page. Complete benchmarks attached.

Please prefix with "(optional)" and explain the default value.

Can we decrypt this by turning form_builder into form_builder() and re-phrasing it accordingly, please? Also, maybe move $children above $sort, since both lines seem to be related to the next block and likewise to the comment block? That said, I needed to read the code, because I was not able to grasp what the comment tried to explain.

Wrong indentation here.

This probably means, $sort is applied recursively - maybe add this to the @param $sort description?

Aside from that, this code does not seem to change much, but has an impact on the performance. Nicely done!

Thanks for the review sun, here's a patch with those changes.

patch which actaully applies this time.

Improved inline docs.

It would be good to be a bit more explicit about what $element['#sorted'] = TRUE does. For example, does it tell the renderer that this element (and its sub-elements) is _already_ sorted and that it could skip sorting it with uasort()? If so, let's explain that a bit better because "more fine-grained control" is reasonably vague still. Thanks!

#rendered was breaking forms quite drastically, which makes sense thinking about it. Stuffing the fully rendered elements into #rendered and returning that early instead helped a bit, but still a few fails. So I've removed that aspect of the patch for now.

Thinking about this more, I also removed the $sort parameter from the function, since we didn't end up using it anywhere in this patch, and Moshe is right that it's the modules providing elements which know if they're sorted or not. Tried to add and clarify the comments a bit, would be good if someone either more or less familiar with drupal_render() than me (which means anyone ;) ) could look over to see if they make sense.

Additionally this is one of three patches changing something in drupal_render() at the moment, and Damien correctly pointed out on irc that we don't have specific unit tests for it - so opened an issue to add base tests for it here: #354999: unit tests for drupal_render() - once there's something there, we can add some extra tests for the sorting stuff here.

Benchmarks with this patch on 30 nodes show we still get the bulk of the performance gain:

HEAD:
6.00 reqs/sec
6.03 reqs/sec
5.89 reqs/sec

Patch:
6.53 reqs/sec
6.48 reqs/sec
6.37 reqs/sec

The last submitted patch failed testing.

"they have not been rendered by form_builder()". form builder does not render. i suggest removing that sentence. lets make not confuse people by mentioning fapi in drupal_render().

i disagree with the #sorted on node links. We are bothering nodeapi modules if we do that. the comment_render one is ++.

node links themselves don't get #sorted, but taxonomy links (which are already sorted by the query which fetches them) do get it. Some of the older patches had #sorted on node_links but I removed it a while ago - since I agree it's useful there.

Removed the form_builder reference which isn't really needed there anyway, also some whitespace changes which crept in since they were offending chx's love for small cuddly animals.

"trying to sort them a second time". just sorting them a second time.

"!isset($elements['#sorted'])". i think you want empty() not isset

i will rtbc after those.

Both changes are good, can't assume no-one is going to set #sorted = FALSE, here's a re-roll.

Committed to CVS HEAD. Thanks catch.

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