Don't invoke hook_url_outbound_alter() if there's nothing to invoke

For background, sun and I are trying to further optimize hook loading and behavior. In testing, we discovered that hook_url_outbound_alter is called 22 times on even a basic simple node view page. While module_implements() is now cached and drupal_alter() has been cleaned up, that's still 44 function calls on every page for an edge case hook that will be used on maybe 1% of all sites. Stack calls in PHP are sadly not free. This patch saves us around 44 useless function calls per page request on a basic page node, a number that will scale linearly with the number of links on the page.

In IRC, sun made the point that module_implements() cannot change mid-request, because code cannot be unloaded. Thus static here is safe. However, that should be documented. The patch looks good to me aside from that.

Good bye, 44 function calls.

"there any" should be "there are any"?

If url() is called before full bootstrap, what happens?

For example: let's imagine that the module_implements cache is not primed, and url() is called before DRUPAL_BOOTSTRAP_FULL. module_implements() will prime the cache based on the bootstrap modules. #fail

I'm not sure what happens, but whatever it is I don't believe it's changed by this patch.

drupal_alter() calls module_implements() right off the bat. So if module_implements() is going to end up getting broken by url() being called to early, this patch won't change it. It just moves the module_implements() call up one level in the function stack. The issue Damien mentions in #7 is a good question, but I believe is independent of this patch.

Yep, right. I wasn't meaning to change the status anyway.

This is a no-brainer. Committed to CVS HEAD.

Same patch was also at #523284: Optimize url() for a week. See discussion there, and #620452: Optimize drupal_static(): we shouldn't sacrifice simpletest coverage for performance and vice versa.

drupal_alter() now invokes alter functions from themes. Plus, simpletests can change which modules are enabled. I don't think there's a good use-case for themes needing to alter outbound urls, and I don't think simpletest needs should overshadow performance needs, but if we can get #619666: Make performance-critical usage of drupal_static() grokkable to a community-accepted state and land, then we can have the same static variable optimization, but be resettable.

#619666: Make performance-critical usage of drupal_static() grokkable is in, so I suggest we roll this back.

100,000 calls to url (no implementations of the hook):

HEAD: 2.9468100071

Patch: 3.12243294716

2.94 vs. 3.1 what? :-) Seconds, milliseconds, or nanoseconds? It still seems like an unnecessary roll back, depending on the unit.

Sorry, total seconds as measured by the timer, for the total of 100,000 iterations.

I'm in favor.

But, on #320331-110: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks, chx was unhappy that introducing the drupal_alter() step created an 8% performance regression for accessing a node that displays 500 links. drupal_alter() is now much more optimized, which is why I'm in favor of patch 15. However, it's still not free. Apache benchmarks are attached for a slightly modified version of chx's test:

function test619566_node_view($node, $build_mode) {
  $output = '';
  for ($i = 0; $i < 500; $i++) {
    $output .= l($i, "node/$i") . '<br />';
  }
  $node->content['test619566'] = array('#markup' => $output);
}

Patch 15 introduces a 2% regression. Much better than the 8% from when drupal_alter() was much slower. On any core Drupal page, the performance hit is much smaller, since this test is extremely biased to spending lots of time in the l() function.

Let's let Crell, chx, and others weigh in on whether the extra stack call to drupal_alter() is acceptable.

I think this is still an issue. I'm not sure if the question from #7 has been answered, and there's renewed talk about url() before full bootstrap in #320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks. If module_implements() can be broken by being called during an unexpected part of the bootstrap sequence, that seems like something that needs an issue opened and dealt with. Otherwise, it looks like module_implements() resets the 'module_hook_info' and 'drupal_alter' static caches, so #15 would remove the bootstrappiness problem caused by the use of a non-resettable static in url().

Looks like #15 causes a 6% slow-down to the execution of url() when nothing is implementing an alter hook. I'm not aware of any core Drupal page that spends more than 10% of the time in the url() function, so that results in a worst case scenario of 0.6% degradation, but usually quite a bit less than that. There might be some use-cases like #19 in contrib, where the degradation is worse due to a page only invoking l() calls and not much else, and even that example caps at 2%.

Thoughts?

Committed to CVS HEAD. Thanks.

Why remove the main code comment?

Committed to CVS HEAD.