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.

CommentFileSizeAuthor
#5 inline-children.patch900 bytesCrell
inline-children.patch898 bytesCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Issue tags: +Performance

Gah, forgot to tag.

Status: Needs review » Needs work

The last submitted patch failed testing.

Crell’s picture

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)
catch’s picture

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.

Crell’s picture

Status: Needs work » Needs review
FileSize
900 bytes

And this time with a patch attached, too!

catch’s picture

Status: Needs review » Reviewed & tested by the community

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

Crell’s picture

Issue tags: +Quick fix

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -Quick fix

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