[6.x] Performance: Optimize site loading with hundreds of sites#14988
Open
lazerg wants to merge 1 commit into
Open
[6.x] Performance: Optimize site loading with hundreds of sites#14988lazerg wants to merge 1 commit into
lazerg wants to merge 1 commit into
Conversation
Member
|
Hey @lazerg, thanks for this! We've noticed that you opened several PRs in a row and while we appreciate this in general, could you please be more elaborate on what the actual changes are in the description? While some of the PRs you've opened are simple, something like this here is more complex. Reviewing PRs can take a lot of time to make sure it actually fixes the underlying issue without introducing new bugs or unforeseen (breaking) changes. Thank you! |
Contributor
Author
|
hi @joshuablum , please recheck the pr desc |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14670.
This comes from the issue, where someone running around 800 sites profiled the control panel and found three spots that scale badly with site count. None of them show up with a handful of sites. The problem is that each one does work proportional to the total number of sites, on paths that run during normal page loads, so the cost climbs fast once you're in the hundreds or thousands.
Here's what each change does, why the old version was slow, and why the new one is safe to swap in.
1.
Nav::existsIn()no longer loads every site's tree to answer one questionexistsIn($site)wastrees()->has($site). The catch:trees()builds the full set by callingin()for every site and dropping the empty ones. So asking "does this nav exist in site X" quietly ran a repository lookup for every site in the install. It's nowin($site) !== null, which looks up only the site you asked about.in()already returns null when a site has no tree, so the boolean is identical. It just gets there without touching the other sites.2.
Sites::authorized()skips the per-site gate check for super usersauthorized()filters the sites withcan('view', $site), one gate call per site. For a super user the answer is always the whole list, becauseSitePolicy::before()returnstruefor them before the policy method even runs. But the gate still pays its per-call cost for every site to reach that answer. The early return hands back the full collection when the current user is a super user. Everyone else falls through to the same filter as before, so their permissions are evaluated exactly the way they were. This only cuts work in the case where the outcome is already decided.3.
Site::resolveAntlersValue()stops running plain strings through AntlersEvery site config value (name, url, locale, and so on) was passed through
Parse::config()and the Antlers runtime, including values likeen_USor/about/that have no Antlers in them. The guard returns the value untouched when it isn't a string, or when it's a string containing neither{nor@. Those are the only two characters that can start Antlers:{{ }}tags and@directives. A string without either can't be a template, so parsing it is wasted work. Anything that does contain Antlers still goes through the parser unchanged, so templated config values keep resolving as before.Tests
Added
SitesPerformanceTestandNavPerformanceTest. They check behavior, not timing, since wall clock is flaky in CI:existsIn()returns the right answer for a single site and across multiple sitesOne thing worth flagging
The issue also points out that the CP nav menu in
CoreNavchecks each nav with$nav->sites()->contains(Site::selected()->handle()), which runs throughtrees()and so hits the same load-everything problem change #1 fixes insideexistsIn(). Right nowexistsIn()is only called by the nav controller, so the CP menu itself doesn't benefit from change #1 until those two call sites switch to$nav->existsIn(...). I left it out to keep this PR narrow. It's a two-line change though, so happy to add it here if you'd rather keep it in one place, or send it as a small follow-up. Your call.