Within the menu system alone there are multiple calls to _menu_load_objects. On one of the config pages for domains it's called 8 times, and every time it calls domain_load. That would be fine except domain_load is not caching any call.
When I test the domain_lookup function, it calls the db every time. This is definitely going to be a performance drag, especially considering it calls hook_domain_load every single time as well.
Wouldn't it be better to just keep a default $reset = FALSE for domain_load?
function domain_load($domain_id = NULL, $reset = FALSE) {
$domain = domain_lookup($domain_id, NULL, TRUE);
if ($domain == -1) {
return FALSE;
}
else {
return $domain;
}
}
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 1983860-do-not-use-domain_load.patch | 8.92 KB | agentrickard |
| #11 | 1983860-do-not-use-domain_load.patch | 8.51 KB | agentrickard |
| #10 | 1983860-do-not-use-domain_load.patch | 6.79 KB | agentrickard |
| #8 | 1983860-do-not-use-domain_load.patch | 2.96 KB | agentrickard |
| #3 | 1983860-do-not-use-domain_load.patch | 1022 bytes | agentrickard |
Comments
Comment #1
agentrickardWith reports like this, please say exactly what page(s) you are referring to.
Then we need benchmarks. As a general rule, the domain_load() function should only be called by the menu router, and caching the result makes no sense, because it should be loading each domain one time.
Comment #2
agentrickardI just did a quick scan through the code. There are two instances where domain_load() should be replaced with a cached domain_lookup().
domain_source_lookup()
domain_theme_form_alter()
In all other cases, it is used because we cannot load a cached data set.
Comment #3
agentrickardI found a version of this in the wild. We need this patch. domain_load() should only be used internally.
Comment #4
agentrickardBumping priority.
Comment #5
asherry commentedSorry I didn't specify a page, I was mainly talking about a url like the theme admin page:
admin/structure/domain/view/[domain_id]/theme
In this case the menu router calls _menu_load_objects around 8 times. I noticed this problem because it then calls domain_api every time, and runs hook_domain_load multiple times on the same domain, which is odd.
With node_load, since it uses the entity controller, it runs once per actual node nid, then you know that hook_node_load will also run once for a particular node. It seems like domain_load should run once per domain, and then read from cache if the domain has already been loaded - like what the default entity controller does.
Comment #6
asherry commentedI'm also really concerned that domain_domains runs on most pages. I'm developing a site now that will have from 50 - 100 domains, and it will kill performance to run domain_lookup on all 50 on each domain request.
Any thoughts? I can spend some time on this, and domain_load itself, I just will need some context.
Comment #7
agentrickardDoes the patch in #3 help? Please review.
Comment #8
agentrickardHere's another version that also removes the domain_domains() call for node lookups.
Comment #9
agentrickardMetrics for the patch, btw, from a live site. Top is "before patch".
http://imgur.com/a/Lu1tb
Comment #10
agentrickardAlright, here's a version that rips out much of the excess loading, and also removes the bad module_implements() reset that causes so much pain.
Needs some testers! Does this break anything?
Comment #11
agentrickardThe previous patch only worked if any module implementing hook_domain_load() also implemented hook_boot() or hook_domain_bootstrap_load().
The solution is to prevent modules from calling domain_lookup() too soon in the bootstrap cycle. This patch should be ready to go.
Comment #12
asherry commentedThat looks good to me, I was testing with adding a hook_domain_load in the domain_theme module and now it only loads it for the appropriate domains - no duplicates.
I do notice though that on the domain list page it runs the domain_api multiple times for the default domain. I wonder if we could change line 1568 to
domain_default(TRUE, FALSE);
It doesn't seem like we need to run the api again if it's already statically cached?
That's so much for this patch.
~Alan~
Comment #13
agentrickardNice catch.
Comment #14
asherry commentedThis works great for me now. I tried out our client's site with 2 domains, and then with 45 domains and didn't notice any difference in load time for pages only pertaining to the current domain.
Comment #15
agentrickardCommitted!
Comment #16.0
(not verified) commentedfixed the code and typo