Remove drupal_render_children() from drupal_render()

Crell - November 2, 2009 - 05:08
Project:Drupal
Version:7.x-dev
Component:base system
Category:task
Priority:normal
Assigned:Crell
Status:closed
Issue tags:Performance, Quick fix
Description

According to the latest benchmarks, the theme system, and the drupal_render pipeline in particular, is one of our larger time sinks. drupal_render() is intensely recursive, and with drupal_render_children() is double-recursive. PHP stack calls are not particularly fast. So let's see if we can save ourselves a few stack calls in drupal_render(), which is along pretty much every critical path now.

The attached patch removes the call to drupal_render_children() from drupal_render() and inlines the same behavior. There should be zero functionality change, but we do save one function call for every time we need to render a child element. I did not remove drupal_render_children() because it's still called from a ton of theme functions, but at least this will reduce the recursion depth.

With APC enabled using ab -n200 -c1 on node 1, Before:

Requests per second:    10.72 [#/sec] (mean)
Time per request:       93.251 [ms] (mean) 
Time per request:       93.251 [ms] (mean, across all concurrent requests)

And After:

Requests per second:    11.07 [#/sec] (mean)
Time per request:       90.373 [ms] (mean)
Time per request:       90.373 [ms] (mean, across all concurrent requests)

Not a massive difference, but every little bit helps. It should also be a bigger difference on sites with more complex node bodies, or deep forms like CCK forms, because those have deeper recursion in the rendering pipeline.

AttachmentSizeStatusTest resultOperations
inline-children.patch898 bytesIdleFailed: Failed to install HEAD.View details | Re-test

#1

Crell - November 2, 2009 - 05:09

Gah, forgot to tag.

#2

System Message - November 2, 2009 - 05:15
Status:needs review» needs work

The last submitted patch failed testing.

#3

Crell - November 2, 2009 - 05:28
Status:needs work» needs review

Sigh. And this time hopefully without stupid bugs I have no business writing.

Before:

Requests per second:    10.83 [#/sec] (mean)
Time per request:       92.294 [ms] (mean)

After:

Requests per second:    11.03 [#/sec] (mean)
Time per request:       90.637 [ms] (mean)

#4

catch - November 2, 2009 - 05:30
Status:needs review» needs work

I profiled locally. On a very simple page, drupal_render_children() is 0.3% of page execution time. That time is (obviously) removed by this patch, but not back into drupal_render(). It's a tiny optimization, but it's a good one, and still very readable.

#5

Crell - November 2, 2009 - 05:46
Status:needs work» needs review

And this time with a patch attached, too!

AttachmentSizeStatusTest resultOperations
inline-children.patch900 bytesIdlePassed: 14671 passes, 0 fails, 0 exceptionsView details | Re-test

#6

catch - November 2, 2009 - 06:27
Status:needs review» reviewed & tested by the community

Very small optimization but it's still quite readable, so a no-brainer.

#7

Crell - November 7, 2009 - 02:48
Issue tags:+Quick fix

Tagging. A < 1 KB patch should not still be sitting in the queue...

#8

Dries - November 7, 2009 - 14:02
Status:reviewed & tested by the community» fixed

Looks good to me, although it is somewhat of a micro-optimization. Committed to CVS HEAD. Thanks!

#9

System Message - November 21, 2009 - 14:10
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.