Problem/Motivation
Visiting node/1 with the standard install, anonymous, no page cache, warm caches, takes 39,000 function calls.
Equivalent Drupal 7 requests are between 5-10,000 function calls.
One of the worst offenders is Drupal::service() - this is called 397 times, and there are stack calls underneath it, so it's responsible for thousands of function calls.
One of the biggest callers of Drupal::service() is t()
One of the biggest callers of t() is system_region_list()
system_region_list() is called three times during the request. It has no static cache. It ends up translation the region names 60 times. 60 calls to t() = ~542 function calls.
We never print the region names except in block admin.
Proposed resolution
Don't translate the fucking human readable region names when we only want the machine names anyway.
Don't know exactly how much this saves, but it's probably 2-3ms per request. However this is for every single request assuming a site has block module enabled.
There's an extra hidden bonus. Once this is removed, we only have 21 calls to t() on this page. If we remove those, we'd also save initializing the translation service, and fetching the locale cache (when not viewing the site in English) - this is very feasible to get to I think.
Remaining tasks
TBD.
User interface changes
None.
API changes
Extra param to system_region_list()
Comment | File | Size | Author |
---|---|---|---|
#45 | system-region-list-2497259-45.patch | 498 bytes | David_Rothstein |
#39 | 2497259-39.patch | 4.69 KB | stefan.r |
#39 | interdiff.txt | 1.07 KB | stefan.r |
#32 | 2497259-32.patch | 4.63 KB | aerozeppelin |
#32 | interdiff-2497259-30-32.txt | 605 bytes | aerozeppelin |
Comments
Comment #1
Wim LeersThat seems a bit much?
… because all translated strings are render cached? Or? (It sounds like you're saying we can literally have zero
t()
calls!)Nit:
string[]
.Comment #2
catchI think it's 2-3ms with xhprof profiling overhead.
Right, we should only translate strings when rendering. So the only calls to t() should be 1. cold render cache 2. silly cases like this.
Uploading xhprof screenshot of the remaining calls to t() - as you can see most of them fall into category #2.
Comment #3
catchThere are several other calls to the actual translation services - but again most of these aren't for strings that will actually get rendered either.
Comment #4
Wim LeersLooking forward to patches for those :)
Comment #6
Wim LeersUnit test failure.
Comment #7
tim.plunkettWFM, but I see no reason to change the name of the method, it's still giving us back names (just machine names).
Comment #8
Wim LeersComment #9
alexpottLooking at this maybe the active theme should also have the regions injected?
Comment #10
alexpottSomething like this... feel free to ignore - but I think it makes sense for the active theme to know about it's regions. No interdiff because it's a different approach.
Comment #11
neclimdulI kinda of like alex's approach. What I don't like is its only relevant to the active theme. I looked at this earlier and there are like 10 calls to system_region_list() and only 3 are looking for the labels. It seems like those other calls can't use this because of the active theme limitation. :(
Comment #12
catchI like #10, moving back to RTBC for that (which I think I can do since it's completely different from my patch). I was a bit shocked when I found this issue that we still had system_region_list() at all, so moving away from it slowly is nice.
Comment #13
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC + 1, the approach in #10 is great, we can still follow-up with more things.
Comment #14
tim.plunkettAlways feels good to do this. +1
Comment #15
catchI crossposted with neclimdul. It would be good to get rid of system_region_list() altogether (or at least to have a 100% replacement for the cases where labels aren't needed), but I think this is a good interim step to fix this issue too. Leaving RTBC a bit longer.
Comment #17
catchCommitted/pushed to 8.0.x, thanks!
Comment #18
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedDid a quick test with Drupal 7 and the same thing applies there - 60 t() calls within system_region_list() on the front page of a standard install. Not sure if it's a significant performance impact, but seems pretty useless either way.
I am guessing the earlier patch in this issue (#7) is the one to look at for backport.
Could have sworn there was already an issue similar to this somewhere actually.... but maybe not.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedFound #1759306: Many unecessary calls to t() for strings that are never shown anywhere ... but it's broader than this one, so no overlap really.
Comment #23
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedComment #24
aerozeppelin CreditAttribution: aerozeppelin commentedPatch for D7. Borrowed from #7.
Comment #26
aerozeppelin CreditAttribution: aerozeppelin commentedFix for failing a test in #24.
Comment #27
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThanks for your work on this!
Looks already really good and keeps BC nicely.
A code comment though:
nit - Why the indentation change here?
Comment #28
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedYour welcome. Thank you for reviewing my patch. :D
Oops! I overlooked the change in indentation. Here is the updated patch.
Comment #29
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThank you for a new patch. It was a pleasure reviewing it again in more detail:
It will need some more work unfortunately as I found some things:
I don't think this change is useful as we already have called the function and need the labels.
Lets leave the old code here.
We will need to update the doxygen as well as now the return value depends on the new parameter.
The array_keys() must be removed.
Comment #30
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commented@Fabianx, Sure thing :D Changes from #29.
Comment #31
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedLittle nit, but setting to RTBC. Looks great to me now!
nit - Maybe: "if $labels is set to FALSE" ?
Comment #32
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedChanges as per #31.
Comment #33
joseph.olstadawesome! thanks
Comment #34
joseph.olstadDid some quick tests on the latest patch #32 , works as expected
I also did some very unsophisticated performance profiling , it seems to shave off a noticeable amount of time off of a cache clear (tested on a site that has a lot of t strings and translations)
I did not notice a difference elsewhere however my performance profiling tests were very basic to the naked eye. I'll take it just for the cache clear improvement.
have only used this patch for a few minutes, so far so good. Using it on 7.50 with several other patches. Note; my test system has an SSD drive, so its very low latency, others might notice an even greater performance improvement.
Comment #35
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#34: Thanks for testing, moving over to Stefan for review / commit.
Comment #36
stefan.r CreditAttribution: stefan.r commentedLooks great! Should we clarify in the system_region_list() docs that you'd want to set $labels = FALSE for performance reasons?
Nit: s/(Optional) boolean/(optional) Boolean/
Comment #37
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#36: Sounds good to add to the docs a little.
Comment #38
stefan.r CreditAttribution: stefan.r commentedback to NR for #36
Comment #39
stefan.r CreditAttribution: stefan.r commentedComment #40
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC, this continues to be marked as ready for commit.
Comment #42
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRace condition
Comment #44
stefan.r CreditAttribution: stefan.r commentedRe-reviewed with Fabianx, and committed and pushed to 7.x, thanks!
Comment #45
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis changes the behavior of the function (albeit in a pretty minor way) since current() returns FALSE on an empty array, not NULL.
Here's a quick followup for that.
Comment #46
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedAlso adding this to CHANGELOG.txt, since it's an API addition with a change record.
Comment #48
stefan.r CreditAttribution: stefan.r commentedWe've manually tested this and this looks to be fine. I don't see how this would go wrong, so RTBC
Comment #50
stefan.r CreditAttribution: stefan.r commented