| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Crell |
| Status: | closed (fixed) |
| Issue tags: | Performance, Quick fix |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| inline-children.patch | 898 bytes | Idle | Failed: Failed to install HEAD. | View details |
Comments
#1
Gah, forgot to tag.
#2
The last submitted patch failed testing.
#3
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
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
And this time with a patch attached, too!
#6
Very small optimization but it's still quite readable, so a no-brainer.
#7
Tagging. A < 1 KB patch should not still be sitting in the queue...
#8
Looks good to me, although it is somewhat of a micro-optimization. Committed to CVS HEAD. Thanks!
#9
Automatically closed -- issue fixed for 2 weeks with no activity.