Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
If we do function_exists() || drupal_function_exists() we shave about 10% off the cost of calling theme(), only about .3% of the request on the front page but there's a lot of pages with more theme() calls than that.
Comment | File | Size | Author |
---|---|---|---|
#27 | drupal-theme-optimization-523682-27.patch | 3.02 KB | effulgentsia |
#27 | theme.php.txt | 2.21 KB | effulgentsia |
#22 | drupal-theme-optimization-523682-22.patch | 4.03 KB | effulgentsia |
#17 | drupal-theme-optimization-523682-17.patch | 3.93 KB | effulgentsia |
#16 | drupal-theme-optimization-523682-16.patch | 2.51 KB | effulgentsia |
Comments
Comment #3
catchComment #4
moshe weitzman CreditAttribution: moshe weitzman commentedThis is really micro-optimization and not desireable, IMO. Lets see what others think. If we want to speed up theme() lets break it up a little so xdebug can guide us better on what parts are chewing time.
Comment #5
FiReaNGeL CreditAttribution: FiReaNGeL commentedPersonally I think that every optimization is good if it doesn't break anything, or uglifies code to a point its not understandable. 10% off a function that is called relatively often per page, why not? As catch pointed out in another thread, all the 'micro optimization' patches shaved off 7% of php time, which is not insignificant. And he just started... catch have all my support!
Comment #6
catchCross-posting since this is on my list of 'things the registry makes slower' #497118: Remove the registry (for functions).
Comment #7
catchComment #8
Crell CreditAttribution: Crell commentedWe definitely need to refactor the theme system to find ways to speed it up, but I don't think we can reasonably get away with that this late in the dev cycle. This is a small but dead-simple optimization along the critical path, so I'm OK with it. We should document that fact, however. A single line comment should be sufficient.
Comment #9
catchAdded a comment. On micro-optimization, 0.3% is 1/300th of page execution time, for around 30 characters of code.
Comment #10
ugerhard CreditAttribution: ugerhard commentedI feel bold :-)
The patch is "harmless" and brings a performance gain. What's not to like?
Comment #11
catchMore aggressive version - since a lot of theme functions get called several times on the same page, maintain a static of which functions we've already checked.
Attached webgrinds of HEAD + both patches.
Comment #12
catchComment #13
moshe weitzman CreditAttribution: moshe weitzman commentedDon't you think PHP itself keeps an in memory array of which functions exist. Why duplicate that with a static? Does its static system work faster?
Comment #14
catchAs far as I can tell (and I've only looked at the cachegrinds, not tried to go through PHP internals or done a lot of research), the difference in most of these patches is that isset() empty(), if, and static aren't function calls, and function_exists(), drupal_static(), defined() etc. are. So it's the cost of calling a function, vs. not calling a function - regardless of how fast the actual function being called is.
The most obvious example of this is #523058: Optimize check_plain() where it /only/ removes a function call without replacing it with anything else, and takes half the cost of check_plain() out, but I think we can assume that static, empty() etc. are more or less unmeasurable, whereas function calls are measurable (at least in webgrind, if not always ab), once you get over 20-30 of them during a request.
We could probably verify the actual speed improvements better with microbenchmarks over 10,000 iterations or something to see exactly how much difference there is, I only have a certain amount of confidence in webgrind's ability to capture this (although it's very consistent). But I'm not really set up to do those, whereas webgrind screenshots are about two minutes.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedNew patch:
Benchmarks using attached test523682 module, navigating to test523682/1 (us = microseconds):
Above tests were on a MacBook (Core 2 Duo, 2.0GHz, MAMP with PHP 5.2.6 and APC).
Comments:
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedHere's a patch that in addition to the above, also addresses the slow call_user_func_array() for function-implemented theme hooks. It's not pretty, but it's commented, and it shaves almost another microsecond (8.66us instead of 9.48us, see benchmarks in comment #16).
@catch: in case you haven't been following #400292: Allow preprocess functions for theme hooks implemented as functions, there's a patch there awaiting someone to allay merlinofchaos's concerns that the feature doesn't add more overhead than the value the feature provides (see comments 7 and 37). Part of what I would like to accomplish with the optimizations here is to more easily justify the extra call to isset() that's needed in that patch. You're trusted as someone who can verify the benchmarks I'm claiming. Please comment on both of these issues. Thanks!
Comment #18
catcheffulgentsia: these changes look good - could you confirm you're running all the comparisons with xdebug disabled? That caught me out a few times.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedconfirmed. no xdebug. baseline MAMP install with APC turned on and memory_limit at 256M. no other customizations.
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedapologies. i did have zend debugger extension loaded. turned it off and am re-running tests. will post when done.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedsame results with zend debugger extension not loaded.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedRe-rolled due to #400292: Allow preprocess functions for theme hooks implemented as functions being committed. This patch also moves where "$hook_clone = $hook" is being done before calling preprocess/process functions to more accurately match what was previously being done with "$args = array(&$variables, $hook)".
Comment #24
webchickOh, testing bot...
Comment #26
catchOK so drupal_function_exists() is dead, which means we're left with the call_user_func_array() optimization. Would be interesting to test that on its own anyway.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedRe-rolled patch to only include:
Attached theme.php for testing. This is a modification of catch's one from http://drupal.org/node/400292#comment-1945102. This modified version tests:
Each one is called in 10 sets of 10,000 iterations each. 10 sets in order to calculate a standard deviation. The resulting number is how many microseconds for a single call (i.e., 100 microseconds = 10,000 iterations in 1 second).
Unpatched theme.inc:
nothing__________theme(username)_____theme(placeholder)____theme(table)
0.216 +/- 0.022___100.28 +/- 1.67______16.52 +/- 0.14________165.53 +/- 1.09
Patched theme.inc:
nothing__________theme(username)_____theme(placeholder)____theme(table)
0.216 +/- 0.013___97.75 +/- 0.84______15.22 +/- 0.14________165.34 +/- 1.38
How exciting is a 1-2 microsecond improvement? Maybe not very, but merlinofchaos seems very interested in keeping the theme system overhead as low as possible, so with that in mind, why not commit this?
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedEverything in this issue has been implemented in or made irrelevant by #572618: All theme functions should take a single argument to make preprocess sane and meaningful. What's the right status to set on this? Duplicate? By Design? Somethine Else?
Comment #29
catchDuplicate works :)
Comment #30
Dave ReidIf there's nothing left for this module to do, then just mark as fixed.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedI suspect #30 was a cross-post with #29. Since catch opened this issue, I'll go with his suggestion. I normally have a tighter internal sense of what "duplicate" and "fixed" mean, and ideally would prefer a new status "no longer relevant". But I'm fine with a looser definition of "duplicate" in the meantime.
Comment #32
Dave ReidSorry about the cross-post. Arg. :)
Comment #33
catch;)
Comment #34
catchJust a note I like duplicate because it suggests there's a reference to another issue which fixed the original issue - even if the issue that fixed it was completely different.
Fixed I try to reserve for when a patch got committed (but not always).
By design usually means "this is how it works, bad luck", and won't fix is "this is a terrible idea". No longer relevant would be more accurate though.
Comment #35
Dave ReidDEAR GOD CROSSPOST Agreed duplicate is a good choice.