Use static-cached variable for drupal_is_front_page()
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | Performance |
I have a site which implements custom_url_rewrite_inbound(). I noticed this function was being called an obscene amount of times for many requests, when what I expected was for it to be called only once per request.
It turns out that drupal_get_normal_path calls custom_url_rewrite_inbound, and the latter is called with every request to drupal_is_front_page. Frankly, I'm not convinced these calls to drupal_is_front_page need to call drupal_get_normal_path, but the code is there so someone probably had a reason.
At any rate, the attached patch caches the front page path, so calls to drupal_get_normal_path and custom_url_rewrite_inbound only happen the first time drupal_is_front_page is called. With this patch applied, custom_url_rewrite_inbound() is called twice per request. Once when it is actually needed, to rewrite the incoming request. And a second time to rewrite the front page path, which as I said earlier I'm not convinced is actually necessary or good.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| front_page.diff | 701 bytes | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
We use the same static caching with nearly every other function in path.inc, so it makes sense to implement it for drupal_is_front_page() as well. With a base install, 10-node front page, and a site mission statement enabled, drupal_is_front_page() is called 16 times! I did some ab benchmarking (-c 2 -n 500) and it did marginally improve performance:
Before patch (current HEAD):
Requests per second: 5.87 [#/sec] (mean)
Time per request: 340.586 [ms] (mean)
Time per request: 170.293 [ms] (mean, across all concurrent requests)
Transfer rate: 90.37 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 1.2 0 26
Processing: 298 339 35.2 334 671
Waiting: 272 302 35.6 304 657
Total: 298 340 35.2 334 671
After patch:
Requests per second: 5.90 [#/sec] (mean)
Time per request: 339.184 [ms] (mean)
Time per request: 169.592 [ms] (mean, across all concurrent requests)
Transfer rate: 90.75 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 1.0 0 23
Processing: 304 338 25.7 335 653
Waiting: 277 301 25.2 303 626
Total: 304 338 25.8 335 653
This is a good find and moving this to the 7.x queue so we can fix there first and then back-port since this won't be an API change.
#2
#3
nice catch! simple patch, all tests pass, RTBC.
#4
The performance improvement will be even more for sites that implement custom url rewrites. Thanks for pushing this through.
#5
@Dave: in #drupal, webchick asked if its possible to add a test for this?
#6
Hmm, I'll work on it. It might take a little workaround to get it to test.
#7
Patch looks good to me but let's wait for a test to emerge (per webchick's request). :)
#8
Test completed and ready for review.
#9
Marked #313123: Statically cache drupal_is_front_page() as a duplicate of this issue.
#10
Fixed a commented section that should have been un-commented.
#11
Small request: can we change 'drupal_is_front_page' in
+ drupal_set_message('drupal_is_front_page: ' . (drupal_is_front_page() ? 'TRUE' : 'FALSE'));to just 'front page'?#12
looking good, more core code with tests. comments on the new test:
+ variable_set('drupal_is_front_page_output', 1);could we have a comment with that?
+ variable_set('site_frontpage', $node_path);+ $this->refreshVariables();
+ $this->assertEqual(variable_get('site_frontpage', 'node'), $node_path, t('Set front page path to %path', array('%path' => $node_path)));
this looks to be testing variable_(set|get) more than the site frontpage functionality. perhaps it would be better to test this via drupal's interface? i.e., create an admin user,
drupalGet('admin/settings/site-information'), etc?#13
Thanks for the review. This should probably be in system.test and not a path.test then if we're going to test the front page admin. Working on another revision!
#14
Revised test that's a part of system.test now. It tests the setting the front page at admin/settings/site-information (and also an invalid front page path). I couldn't resist doing a little cleanup on system.test as well, so forgive me. As for #11, I changed it to drupal_set_message(t('On front page.')); because on the default front page with no nodes, the "get started with your install" text has the string "front page" in it.
#15
Had a line with whitespace in last patch.
#16
Found a little bug while trying drupalPost-ing to admin/settings/site-information. Please don't commit this until #343765: DrupalWebTestCase does not set required site_mail variable during setUp() is committed. I will provided a revised patch once that is committed.
#17
Nevermind... #343765: DrupalWebTestCase does not set required site_mail variable during setUp() is marked as "won't fix" now.
#18
Also note the patch in #15 has passed testing-bot, the results have failed to be posted back here:
http://testing.drupal.org/pifr/file/1/2201
#19
Revised de-meowed test.
#20
The last submitted patch failed testing.
#21
Revised patch now that #343765: DrupalWebTestCase does not set required site_mail variable during setUp() landed in core.
#22
Committed to CVS HEAD. Thanks!
#23
Automatically closed -- issue fixed for two weeks with no activity.
#24
Can we port back this to 6.x ? It is a performance improvement, doesn't break any API, and is a simple patch.
#25
Cleaned patch from added whitespace. Would be nice to see this fixed in D6, too.