Use static-cached variable for drupal_is_front_page() [#340557]

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.

Comment	File	Size	Author
#25	drupal_is_front_page-D6.patch	1.04 KB	smk-ka
#24	6.x-static-cache-drupal_is_front_page-340557-24.patch	1.15 KB	dropcube
#21	340557-drupal-is-frontpage-D7.patch	4.72 KB	dave reid
#19	340557-drupal-is-frontpage-D7.patch	4.72 KB	dave reid
#15	340557-drupal-is-frontpage-D7.patch	8.18 KB	dave reid
#14	340557-drupal-is-frontpage-D7.patch	8.18 KB	dave reid
#10	340557-drupal-is-frontpage-D7.patch	3.7 KB	dave reid
#8	340557-drupal-is-frontpage-D7.patch	3.71 KB	dave reid
#1	340557-drupal-is-frontpage-D7.patch	1.23 KB	dave reid
	front_page.diff	701 bytes	Dave Cohen

Comments

Comment #1

dave reid

he/him

English

Nebraska USA

commented 29 November 2008 at 18:34

Version:	6.x-dev	» 7.x-dev
Assigned:	Unassigned	» dave reid
Category:	bug	» task
Status:	Active	» Needs review

Status	File	Size
new	340557-drupal-is-frontpage-D7.patch	1.23 KB

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.

Comment #2

dave reid

he/him

English

Nebraska USA

commented 29 November 2008 at 18:35

Title:

drupal_is_front_page calls drupal_get_normal_path too often

» Use static-cached variable for drupal_is_front_page()

Comment #3

Anonymous (not verified) commented 30 November 2008 at 00:09

Status:

Needs review

» Reviewed & tested by the community

nice catch! simple patch, all tests pass, RTBC.

Comment #4

Dave Cohen commented 1 December 2008 at 08:14

The performance improvement will be even more for sites that implement custom url rewrites. Thanks for pushing this through.

Comment #5

Anonymous (not verified) commented 1 December 2008 at 08:53

@Dave: in #drupal, webchick asked if its possible to add a test for this?

Comment #6

dave reid

he/him

English

Nebraska USA

commented 1 December 2008 at 19:18

Hmm, I'll work on it. It might take a little workaround to get it to test.

Comment #7

dries commented 2 December 2008 at 20:46

Status:

Reviewed & tested by the community

» Needs work

Patch looks good to me but let's wait for a test to emerge (per webchick's request). :)

Comment #8

dave reid

he/him

English

Nebraska USA

commented 6 December 2008 at 18:07

Status:

Needs work

» Needs review

Status	File	Size
new	340557-drupal-is-frontpage-D7.patch	3.71 KB

Test completed and ready for review.

Comment #9

dave reid

he/him

English

Nebraska USA

commented 6 December 2008 at 18:10

Marked #313123: Statically cache drupal_is_front_page() as a duplicate of this issue.

Comment #10

dave reid

he/him

English

Nebraska USA

commented 6 December 2008 at 18:31

Status	File	Size
new	340557-drupal-is-frontpage-D7.patch	3.7 KB

Fixed a commented section that should have been un-commented.

Comment #11

dries commented 6 December 2008 at 21:30

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'?

Comment #12

Anonymous (not verified) commented 6 December 2008 at 21:49

Status:

Needs review

» Needs work

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?

Comment #13

dave reid

he/him

English

Nebraska USA

commented 6 December 2008 at 22:22

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!

Comment #14

dave reid

he/him

English

Nebraska USA

commented 6 December 2008 at 23:41

Status:

Needs work

» Needs review

Status	File	Size
new	340557-drupal-is-frontpage-D7.patch	8.18 KB

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.

Comment #15

dave reid

he/him

English

Nebraska USA

commented 6 December 2008 at 23:53

Status	File	Size
new	340557-drupal-is-frontpage-D7.patch	8.18 KB

Had a line with whitespace in last patch.

Comment #16

dave reid

he/him

English

Nebraska USA

commented 7 December 2008 at 00:36

Status:

Needs review

» Needs work

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.