Improve perceived performance by preloading the template parts defined in Theme JSON when in Site Editor #48924
Conversation
getdave
added
[Type] Performance
|
Related #48683 |
getdave
changed the title
|
Thanks for the review @tyxla. I take your points and they were my main concerns when raising this PR (see description). It comes down to the same as the other PR for Navigation menus. Does the benefit for the majority outweight any downside for the minority. My guess is that this PR will end up being closed as it's too un-targetted to be palatable. |
I fully agree with that, and from my perspective, I can't really tell, because my experience with template parts and themes isn't extensive enough. I just wanted to bring up what I know and would prefer to leave the final decision to someone who can better weigh the tradeoffs you outlined. |
|
Thanks @tyxla. I appreciate you providing that perspective. It's important. I'm going to try one more thing to see if we can improve this implementation. |
| @@ -19,17 +19,26 @@ function gutenberg_preload_template_parts( $preload_paths, $context ) { | |||
| return $preload_paths; | |||
| } | |||
|
|
|||
| $theme_slug = wp_get_theme()->get_stylesheet(); | |||
| // Get any template parts defined in theme.json. | |||
| $theme_json_template_parts = WP_Theme_JSON_Resolver::get_merged_data()->get_template_parts(); | |||
There was a problem hiding this comment.
@tyxla What do you think about this more selective approach? Here we only preload those template parts that are defined by the Theme in the Theme JSON.
There was a problem hiding this comment.
Example
"templateParts": [
{
"area": "header",
"name": "header",
"title": "Header"
},
{
"area": "footer",
"name": "footer",
"title": "Footer"
},
{
"area": "uncategorized",
"name": "comments",
"title": "Comments Template Part"
},
{
"area": "uncategorized",
"name": "post-meta",
"title": "Post Meta"
}
]
There was a problem hiding this comment.
@scruffian I don't really understand the value of having a templateParts definition in Theme JSON but Themes are doing it. It could act as a optimising with the Theme "telling" the editor how to optimise itself.
There was a problem hiding this comment.
This looks like a sensible approach 👍
I assume that WP_Theme_JSON_Resolver::get_merged_data() caches data in runtime so it doesn't have to be processed on every call?
Have you been able to measure its performance impact? It seems to be that it would load pretty quickly because the theme.json data has already been parsed and is available in the memory, but I wanted to double-check.
There was a problem hiding this comment.
I'm exploring both the built in Gutenberg perf tests npm run test:performance
...and also
https://github.com/GoogleChromeLabs/wpp-research/tree/main
...on the advice of @adamsilverstein.
There was a problem hiding this comment.
Performance test results from CI running on this PR (having rebased against trunk):
>> site-editor
┌──────────────────────┬──────────────────────────────────────────┬──────────────┐
│ (index) │ f060a497fa86698a5f7e9bc92d168a8a71225fad │ trunk │
├──────────────────────┼──────────────────────────────────────────┼──────────────┤
│ serverResponse │ '138.1 ms' │ '140.7 ms' │
│ firstPaint │ '482.2 ms' │ '400.7 ms' │
│ domContentLoaded │ '789.2 ms' │ '689.8 ms' │
│ loaded │ '790.5 ms' │ '690.93 ms' │
│ firstContentfulPaint │ '879.53 ms' │ '782.9 ms' │
│ firstBlock │ '1972.53 ms' │ '1872.07 ms' │
│ type │ '74.3 ms' │ '70.8 ms' │
│ minType │ '62.94 ms' │ '60.88 ms' │
│ maxType │ '496.34 ms' │ '148.81 ms' │
└──────────────────────┴──────────────────────────────────────────┴──────────────┘
serverResponse is the most important I believe.
Can also be run locally with npm run test:performance.
There was a problem hiding this comment.
I've reached out in the #core-performance channel on WordPress Slack for some support here. I know @felixarntz was involved in a previous discussion about preloading media so perhaps he has a view?
There was a problem hiding this comment.
I haven't manually tried this change, though running the performance tests locally this PR performs worse in every metric but typing 🤔
- this PR:
- trunk
There was a problem hiding this comment.
I now have tested this locally by throttling to "slow 3g" and loading http://localhost:8889/wp-admin/site-editor.php (which means loading "browse mode", not the site editor in edit mode).
In that setup, the canvas contents take ages to load. Within the canvas, the components that took the most time were navigation and query block. The header template was shown almost immediately, both in trunk and this PR.
Perhaps the stats are a bit misleading for this change, because none measure "everything (or the header) has been rendered". First Contentful Paint is probably rendering the site title "Mindblown: a blog about philosophy". I don't see that we track Largest Contentful Paint, though given the importance of the header for TT3 in the whole template, I'd expect it to be affected by this change.
TLDR: I don't see any positive effect of this change. However! If other people are, I don't want to block this. I'll leave space for others to comment.
There was a problem hiding this comment.
Sounds like you're seeing different things to me. I'll refine the testing criteria and advise further here.
|
Flaky tests detected in ef95d35. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4439126677
|
| @@ -19,17 +19,26 @@ function gutenberg_preload_template_parts( $preload_paths, $context ) { | |||
| return $preload_paths; | |||
| } | |||
|
|
|||
| $theme_slug = wp_get_theme()->get_stylesheet(); | |||
| // Get any template parts defined in theme.json. | |||
| $theme_json_template_parts = WP_Theme_JSON_Resolver::get_merged_data()->get_template_parts(); | |||
There was a problem hiding this comment.
This looks like a sensible approach 👍
I assume that WP_Theme_JSON_Resolver::get_merged_data() caches data in runtime so it doesn't have to be processed on every call?
Have you been able to measure its performance impact? It seems to be that it would load pretty quickly because the theme.json data has already been parsed and is available in the memory, but I wanted to double-check.
getdave
force-pushed
the
try/preload-common-template-parts-editor-loading
branch
from
4d0148e to
1f27278
Compare
getdave
changed the title
|
Looping in @youknowriad here as lead architect in case he has a strong opinion on whether preloading only the templateParts defined in the Theme JSON would be a fair compromise to help improve the perceived loading performance of the Site Editor. |
| } | ||
|
|
||
| // Get any template parts defined in theme.json. | ||
| $theme_json_template_parts = WP_Theme_JSON_Resolver::get_merged_data()->get_template_parts(); |
There was a problem hiding this comment.
@anton-vlasenko @ironprogrammer Do you have any idea of how I might be able to mock WP_Theme_JSON_Resolver in a test? I want to write tests but not sure how I could make that testable.
Yeah, I think that's a good starting point but I'm curious about themes that define multiple headers and multiple footers... Is there a way to get only one header, one footer and one navigation menu (the main ones) |
Some things I've thought about but not explored:
However, I'm mindful of the trade offs for these optimisations. If we're making more queries to avoid making queries for all templates then it seems counter intuative. That said, if there is some heuristic that can allows us to determine the "main" header/footer then that would be ideal. I'm just not sure there's anything to go on. |
|
@youknowriad I wonder if we should also load any template parts that either:
AND/OR
This would involve adding another item to the preload paths along these lines: Reason? The current implementation will not preload custom templates created by the user in the editor unless they edited one of the Theme's template parts. |
|
Yeah, it's not clear, I guess if there's only a couple (or a max threshold we fix of template areas assigned to the "header" and "footer" areas, it should be fine. But I can see a world where some sites downloads a high number of headers and footers and the preloading becomes a problem. So maybe limit this to the first two headers and footers or something like that. |
|
Research
As such preloading |
| } | ||
|
|
||
| // Get any template parts defined in theme.json. | ||
| $theme_json_template_parts = WP_Theme_JSON_Resolver::get_merged_data()->get_template_parts(); |
There was a problem hiding this comment.
In the plugin, we need to use the classes bundled by Gutenberg: WP_Theme_JSON_Resolver_Gutenberg instead of WP_Theme_JSON_Resolver.
There was a problem hiding this comment.
@oandregal On another note it would be nice if there was a stable global function which I could use instead of ::get_merged_data()->get_template_parts(). Is that a good idea?
There was a problem hiding this comment.
Absolutely! We should not use the private classes in places other than the high-level functions (get_global_settings, etc.). It's tracked as part of #45171 (though it's up for anyone to pick it up):
Implement wp_get_custom_templates_from_theme: access to customTemplates defined by the theme
I've updated the PR to only preload the first So we
How do you feel about this now? |
Co-authored-by: Marin Atanasov <[email protected]>
getdave
force-pushed
the
try/preload-common-template-parts-editor-loading
branch
from
3847f23 to
72336e2
Compare
|
Ok folks this is very strange - the original loading problem I was seeing is now not reproduceable. It's possible something changed underneath me in which case great because this no longer seems to be an issue. I'm going to refocus my attention at the Nav block specific loading issues. |
|
Size Change: 0 B Total Size: 1.34 MB ℹ️ View Unchanged
|
What?
Improves the perceived loading performance of the Site Editor by (selectively) preloading template parts (see
Howfor details).Why?
Most of the perceived sluggish-ness of the Site Editor loading is due to having to wait for template parts to resolve from the network.
For example on TT3, when the editor loads requests are dispatched for
/wp/v2/template-parts/twentytwentythree//header?context=edit/wp/v2/template-parts/twentytwentythree//footer?context=editThe former is particularly jarring as the whole header area becomes a loading spinner until the network resolves. This is very noticeable on slower connections (take a minute to test it).
This is further compounded by the fact that most
headertemplate parts will contain a Navigation block which has it's own loading state which is dependent on loading entities over the network. As the Nav block will not be rendered in the editor until the template part resolves, this means we get a loading waterfall effect where we seeThis is not a great experience.
How?
By preloading requests for the templates parts defined by the
theme.jsonwe can ensure these parts are available immediately in the@wordpress/api-fetchnetwork cache upon editor load.This means the editor never dispatches the network
fetchrequest and instead [core data] immediately returns the entity data for the template part directly.This results in almost no loading spinners in the header. When combined with #48683 this should have a major impact on improve the perceived performance.
### Criteria for Preloading a Template
When determining which template parts will be preloaded the following criteria apply:
theme.jsonunder thetemplatePartskey.areaproperty set toheader- these are templates being specifically denoted as being designed for the "Header" Template Area of a site.gutenberg_template_part_preloading_limitfilter.may result in 404 requests on the server if theconcern no longer appliesheaderorfooterparts do not exist.Testing Instructions
trunkwhere there are loading spinners which take a long time to resolve.Testing Instructions for Keyboard
Screenshots or screencast
Before
Notice the loading spinner for the template part.
Screen.Capture.on.2023-03-08.at.12-48-13.mp4
After
Notice no spinner for the template part.
Screen.Capture.on.2023-03-08.at.12-47-13.mp4