Removed drupal_static() for a check which never needs to be reset, added a couple of new statics to replace function_exists(), and access $GLOBALS['base_url'] directly instead of the base_url() wrapper.

On a node with 300 comments, patch saves approximately 8,000 function calls, and 3% of page execution time according to webgrind.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c960657’s picture

Status: Needs review » Needs work

Static caching of clean_url was removed in #356721: Regression: Test failures with clean URLs due to problems with simpletest testing.

catch’s picture

Status: Needs work » Needs review

The benchmarks in that issue reckoned about a 1.5ms hit with 100 calls to url(), this page from standard HEAD + devel generate has 1,600 calls - which is about 15ms if those are right. I don't think we should introduce measurable performance regressions in core just to make simpletest happy, not when we're already significantly slower than D6 was (which in turn was slower than D5) in terms of PHP overhead.

catch’s picture

FileSize
2.52 KB

Let's just access $conf['clean_url'] directly then.

Dries’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Marking RTBC.

catch’s picture

FileSize
3.13 KB
416.71 KB
423.56 KB
3.72 KB

Also remove a function call from drupal_encode_path() (which is only ever called by url()) - makes drupal_encode_path() 25% faster according to webgrind.

catch’s picture

Status: Reviewed & tested by the community » Needs review
catch’s picture

FileSize
219.53 KB
233.02 KB
2.98 KB

Now moving the code from drupal_encode_path() into url(). Between the reduced function calls in url() and not calling drupal_encode_path() at all, we take off 5% of page execution time as measured by webgrind. url() itself is 1/3rd faster.

Benchmarks with ab comes out at 3.2% improvement going by time per request, I'd like to find a reliable way to measure CPU usage itself, but that would require a clean benchmarking environment, which I don't have.

Side note, the raw cachegrind file for HEAD is 16mb, cachegrind for the patch is 14MB, so in terms of raw function calls we're removing a lot.

HEAD:

Document Path:          /node/6
Document Length:        685615 bytes

Concurrency Level:      1
Time taken for tests:   784.294 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Total transferred:      343040000 bytes
HTML transferred:       342807500 bytes
Requests per second:    0.64 [#/sec] (mean)
Time per request:       1568.588 [ms] (mean)
Time per request:       1568.588 [ms] (mean, across all concurrent requests)
Transfer rate:          427.14 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:  1471 1568  56.4   1561    1891
Waiting:     1458 1555  56.2   1548    1877
Total:       1471 1569  56.4   1561    1891

Patch:

Concurrency Level:      1
Time taken for tests:   759.552 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Total transferred:      343040000 bytes
HTML transferred:       342807500 bytes
Requests per second:    0.66 [#/sec] (mean)
Time per request:       1519.103 [ms] (mean)
Time per request:       1519.103 [ms] (mean, across all concurrent requests)
Transfer rate:          441.05 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:  1441 1519  60.8   1504    1939
Waiting:     1428 1505  60.5   1491    1926
Total:       1441 1519  60.8   1504    1939
moshe weitzman’s picture

I don't think we should introduce measurable performance regressions in core just to make simpletest happy

Thats inaccurate, IMO. We remove statics in order to keep a flexible API. All these statics add ssumptions about how we are using drupal. Sometimes people switch users and drupal_execute() and do all sorts of things that a CMS app does not normally do.

catch’s picture

@moshe, what in this patch breaks switching users or using drupal_execute()?

I've found a bunch of issues where 'harmless' changes to make simpletest happy have done anything between costing an extra 2-3% (add that up across 4-5 issues and that's the same kind of gains we were looking at from caching node_load()), to in #537056: Broken drupal_static() conversion in theme_comment_post_forbidden() breaking static caching completely because they weren't properly benchmarked or tested at the time.

Assuming these patches don't break the tests we have, then they shouldn't be introducing edge cases, if we find that they do at some unspecified point in time, then they're easy to revert.

moshe weitzman’s picture

Oops, this patch does not depend on active user. But it does depend on active language. It is quite plausible that a script wants to change languages during its lifetime.

I'm not trying to be a royal pain with these performance patches. I also want D7 to fly. I'm pointing out drawbacks to the patch which I personally think make them undesireable. If the committers disagree, then lets commit them. Ultimately, they should decide.

catch’s picture

FileSize
3.03 KB

It doesn't depend on active language, but that's a stupid variable name I introduced, here's a more self-documenting one.

moshe weitzman’s picture

I have no specific objection to this patch.

jrchamp’s picture

Status: Needs review » Reviewed & tested by the community

Dries marked this RTBC in #4. The only logical difference is that this patch duplicates some code (referencing the original) from drupal_encode_path, rather than incur the function overhead in this critical path. Looks good to me.

catch’s picture

FileSize
904 bytes

Turns out xdebug has been messing with numbers of some of these, so did microbenchmarks with no xdebug enabled just to verify the relative changes within url() itself. Script attached, 100,000 iterations.

HEAD:
nothing: 0.0217690467834 seconds function no_op(): 0.119976997375 seconds url: 7.15640306473

Patch:
nothing: 0.0209450721741 seconds function no_op(): 0.124289989471 seconds url: 5.68390989304

Saves about 7ms on a page with 500 calls to url() so in this case xdebug isn't misleading.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

jrchamp’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.77 KB

Rerolling against HEAD.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

jrchamp’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.44 KB

Rerolling against HEAD. A small portion of this patch appears to have been committed in #578520: Make $query in url() only accept an array, which led to the patch application error.

@catch, @Dries: Is this patch still wanted?

catch’s picture

I measured a 20% performance improvement here within URL, and while the patch isn't the prettiest in the world, I don't think it makes URL 20% uglier either, so IMO it's a fine trade-off.

effulgentsia’s picture

Question for the performance freaks here: if this patch can be shown to result in a better performance gain to page requests than #318636: Make l() themable results in a performance loss, would you support #318636: Make l() themable? Personally, I'd love to see a long-standing Drupal WTF (why can my theme change just about every piece of html that drupal outputs, oh EXCEPT LINKS?) removed in D7. But, I recognize that D7 performance is also a key part to it being adopted as a platform. So, what do you think? Please share your thoughts on that issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

This patch no longer applies due to #320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks. Is some optimization along these lines still possible, given that issue landing? I haven't taken the time to look in detail yet, but maybe someone else following this issue might have a quick answer.

catch’s picture

Status: Needs work » Needs review
FileSize
2.19 KB
1.48 KB

Here's a start on a re-roll, might be some other spots we could optimize, but I measure an improvement in microbenchmarks:

Test script calls url($text, $path) 1 million times, xdebug extension off, APC on.

HEAD:
nothing: 0.266651153564 seconds
function no_op(): 0.605406999588 seconds
url: 41.9536850452 seconds

Patch:
nothing: 0.252546072006 seconds
function no_op(): 0.59984087944 seconds
url: 38.1484689713 seconds

Also a more aggressive version which caches whether we have any hook_url_outbound_alter() implementations. Likely a no-go due to simpletest but see if it passes.

Patch with static for $hook:
nothing: 0.232038974762 seconds
function no_op(): 0.586697816849 seconds
url: 30.4486300945 seconds

Dave Reid’s picture

Yeah, we need static resetting in SimpleTest before we can cache the hook. It won't reliably work otherwise in testing.

catch’s picture

FileSize
906 bytes

If we use drupal_static(), there's likely no performance gain anyway, or not as much anyway. However we have tests for custom_url_outbound() in HEAD I assume, and this patch doesn't break them, so something to be said for that.

Benchmark script attached. You need to benchmark with and without HEAD patched.

Dave Reid’s picture

Odd. The tests call url() and drupal_get_normal_path() directly, so any static saved information from normal site is carried over into the test enviornment. Not sure why it passed the bot. :/

effulgentsia’s picture

Seems like some other issue (not sure which one) implemented a non-resettable static to mitigate the performance impact of the drupal_alter(). I'm suspicious of this. Please see #620452: Optimize drupal_static(): we shouldn't sacrifice simpletest coverage for performance and vice versa.

catch’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

re-upping #23 for the bot.

casey requested that failed test be re-tested.

catch’s picture

Status: Needs review » Closed (won't fix)

#619666: Make performance-critical usage of drupal_static() grokkable is in, so that only leaves base_path() and in-lining drupal_encode_path() - neither of which are going to gain us anything much, so I'm closing this, since all the important stuff went in elsewhere.