Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Let's save hundreds of senseless function calls.
Comment | File | Size | Author |
---|---|---|---|
#23 | url-restore-code-comment.patch | 629 bytes | David_Rothstein |
#19 | ab-url-619566-15.txt | 2.72 KB | effulgentsia |
#15 | url.patch | 1.27 KB | catch |
#6 | drupal.perf-url-outbound-alter.6.patch | 1.21 KB | sun |
#2 | drupal.perf-url-outbound-alter.2.patch | 1.21 KB | sun |
Comments
Comment #1
Crell CreditAttribution: Crell commentedFor 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.
Comment #2
sunMore docs + critical fix :-D
Comment #3
sunNot sure how long that post will remain, but: http://drupalbin.com/12155 contains a list of hook invocations on node/1. hook_url_outbound_alter() is #1.
Comment #4
Crell CreditAttribution: Crell commentedGood bye, 44 function calls.
Comment #5
Dries CreditAttribution: Dries commented"there any" should be "there are any"?
Comment #6
sunheh, yeah :) I KNEW I missed some word in there. ;)
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf 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
Comment #8
sunSorry, but when is that the case?
If that is a valid scenario at all, then I'd say that we need a test that ensures this functionality is possible at all.
Comment #9
Crell CreditAttribution: Crell commentedI'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.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedYep, right. I wasn't meaning to change the status anyway.
Comment #11
Dries CreditAttribution: Dries commentedThis is a no-brainer. Committed to CVS HEAD.
Comment #12
catchSame 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.
Comment #13
effulgentsia CreditAttribution: effulgentsia commenteddrupal_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.
Comment #15
catch#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
Comment #16
Crell CreditAttribution: Crell commented2.94 vs. 3.1 what? :-) Seconds, milliseconds, or nanoseconds? It still seems like an unnecessary roll back, depending on the unit.
Comment #17
catchSorry, total seconds as measured by the timer, for the total of 100,000 iterations.
Comment #18
sunComment #19
effulgentsia CreditAttribution: effulgentsia commentedI'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:
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.
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedI 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?
Comment #21
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedWhy remove the main code comment?
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedComment #24
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.