Performance Chat Summary: 9 May 2023
Meeting agenda here and the full chat log is available beginning here on Slack.
Announcements
- n/a
Priority Projects
Server Response Time
Contributors: @joemcgill @spacedmonkey @aristath
- @spacedmonkey has a number of PRs that are nearly ready to commit:
- @joemcgill I’m waiting on some final feedback, but am close to being able to share the results of the performance research I finished up a few weeks ago. The highlights from that research are that there may be some good opportunities to look into the way template rendering is being handled in both classic and block themes, and also some bottlenecks I observed with translations in classic themes
- @swissspidy Curious about the translations part and how it only affects classic themes
- @joemcgill Same! I noticed a very large difference between the two architectures. Not sure why that is happening, so it would be really useful to look into.
- @spacedmonkey This is a follow on from get_pages -> get_posts https://github.com/WordPress/wordpress-develop/pull/4386 – Add filter to allow developers to change params to WP_Query, an ask from ployglot
- @spacedmonkey I opened this as well [58277] TLDR, saves around 600+ calls to wp_cache_get that are not needed
- Numbers are very very promising in blackfire but not as much in benchmarks, still working this out
Database Optimization
Contributors: @aristath @spacedmonkey @olliejones @rjasdfiii
- @spacedmonkey updates above
- @olliejones I’ve been working on object cache instrumentation, no big results to report
- @spacedmonkey There a project afoot to add a spec for getting object cache information https://github.com/rhubarbgroup/wp-object-cache-info-spec
- @olliejones A new revision of this is coming soon, from me
- @spacedmonkey Basically add a
wp_cache_infofunction to core and allow WP CLI, query monitor and other tools to understand the cache implementation
JavaScript & CSS
Contributors: @mukesh27 @10upsimon @adamsilverstein
- One agenda item here today Discussion this week on CSP/async concerns in https://github.com/WordPress/wordpress-develop/pull/4391
- @westonruter I left some more feedback related to compatibility with Content-Security-Policy and the deferred inline scripts: the current implementation has a vulnerability related to CSP nonces, and it also blocks Strict CSP by requiring
’unsafe-hashes’due to theonloadattribute. I provided recommendations for how to address these issues and have been discussing with @joemcgill - @joemcgill One of my questions about the CSP concerns you raised, @westonruter is whether this whole class of problem would be eliminated if we were to change the way we are approaching
asyncdependencies, or if the problem exists withdeferas well? For example, if we weren’t concerned about maintaining loading order (because withasyncit shouldn’t matter if the scripts are well constructed) then we wouldn’t need to control loading inline scripts so they run after their dependencies. - @westonruter Yes, the problem with CSP nonces exists for deferred inline scripts for
deferscripts as well - @joemcgill Ok, so I’ll need to double check whether it still makes sense for us to control the loading order there, and if so, then work on implementing one of your suggestions. Your explanation and research here has been really valuable, so thank you!
- @westonruter If we eliminate special support for deferred inline
afterscripts forasyncscripts, then we can get rid of theonloadattribute and just useaddEventListener('load')in anafterscript. - @flixos90 Being unfamiliar with “it also blocks Strict CSP by requiring
’unsafe-hashes’due to theonloadattribute” can you elaborate or share a link with how theonloadattribute causes a problem there?” - @westonruter sure, CSP’s security model relies on nonces and hashes. For nonces, all
scriptelements on a page should have anonceattribute which matches anoncevalue passed in theContent-Security-Policyresponse header. However, such nonces are not possible to be attached to event handler attributes, likeonload - @flixos90 Your suggestion with
addEventListener('load')is problematic if it is wrapped around the inline script because it changes the scope of the code within. This was ruled out as an option early on in the design of this API because of that - @westonruter Actually not quite because the callback passed to
addEventListener('load')still just callswpLoadAfterScripts(handle)In the same way thatonload="wpLoadAfterScripts(handle)"is now - @flixos90 Ah, so you mean WP core would handle that event listener on its own?
- @westonruter It’s just that this change is not compatible with
asyncscripts, but it does work fordeferscripts - @flixos90 That is the part I don’t understand, why wouldn’t that work for
asyncwhereasonloadwould? - @westonruter Because
asyncscripts can load immediately before the rest of the document loads. Example: https://async-script-load-event-listener-test.glitch.me/?delay=500 - @flixos90 I think the approach of the event listener is definitely preferable, as
deferis more important to holistically support at this point, as it works in a simpler way with the current way core handles dependencies- That would mean we keep
deferfunctioning like it does now, but will have to (continue to) rethinkasyncsupport, which I think is okay. I’d also like to throw out there a consideration of potentially only launching withdeferin the first version of this - That would give us time to decouple the
asyncexploration
- That would mean we keep
- @joemcgill Apart from getting into the weeds about how to best address the CSP issue, I do think it’s worth a broader discussion about whether our current approach to
asyncscripts is too conservative (mostly to try and help developers avoid loading errors with dependencies). - @joemcgill I think the concerns we based the current implementation from are valid, but maybe a bit heavy handed, so I’m open to us being less concerned with load order concerns if everyone is agreed.
- @westonruter
- My impression is that
asyncscripts are currently quite rare compared todeferscripts. It’s even rarer thatasyncscripts would depend on each other. But in the cases where they do have dependencies (e.g. AMP) then the loading order is handled in the scripts themselves, and not by source order in the page. Other cases of async scripts having dependencies, e.g. Google Analytics, it’s also handled by the library and loading order doesn’t matter - @flixos90 My concern with loosening that is around developer education. We would need to be very clear on what applying
asyncto a script implies. Core would handledeferto always work (make it blocking if needed), but forasyncit would indeed be on the plugin / theme developer if we loosen the current limits - @joemcgill I think that it would be a reasonable position to take, that if you are registering a script that is
asyncor depends on anasyncscript, then it is your responsibility to manage the load order concerns. But agree that dev ed would be important - @spacedmonkey FYI, gutenberg will likely needed deferred scripts for Interactivity API runtime ( I guess is part of co-op editting ). See https://github.com/WordPress/gutenberg/pull/49994
- @westonruter Fortunately, there are ample educational materials out there for how
asyncbehaves. And devs should only use it if they know what they are doing.- @flixos90 That’s what concerns me
- @joemcgill Yep! But as this would be somewhat new for WP script enqueueing, we should support that with some WP-focused documentation as well.
- @joemcgill Unless someone feel strongly that we should NOT make this the responsibility of the developer, @10upsimon and I can work on updating the current approach to allow for a less strict implementation.
- @flixos90 I think this still deserves more discussion, also with the other now additional consideration that inline scripts would also be responsibility of the developer
- @westonruter What’s certain is that while we would promote
defer, we would need to be very clear in the docs thatasyncorder is not predictable. So whiledefercan often be applied to all scripts, this is definitely not the case forasync. - @joemcgill Happy to make it a future agenda item here. We’ll also be looking for feedback when we post on make core about the feature and can address it there as well.
- @flixos90 But more importantly, what about decoupling this discussion? I suggested that above: I think we’re all pretty clear what should happen with
defer, but we need to discussasyncmore- @joemcgill agreed
- @flixos90 Why not remove all
asyncsupport from the current PR and keep that for an iteration? This allows us to iterate on what is already clear, and gives us time to approach the other problem separately - @joemcgill I think we should save that option for if we can’t come to resolution in the next few weeks.
- @flixos90 That’s fair, though I want to make sure we don’t come to early conclusions and eagerly update the PR with something that is then still being discussed
- @flixos90 It’s clear that we’re basically “going back to the drawing board” with
async. So one could argue that topic is not yet ready for engineering. Having that discussion and providing related comments on the PR makes it more difficult for the folks working on the code to focus on the improvements to the PR that are clear- @joemcgill I disagree that we’re going back to the drawing board, but am happy to pick this up async so we can move on with this conversation
- @flixos90 We are changing the design of how
asyncsupport works from how it was designed, that’s what I mean by that. Which of course is not a bad thing, but we’re not ready to write code for that because of it still being discussed - @joemcgill Yeah, but it seems like a rather small change that will result in reducing the logic we’ve implemented, not adding a lot more, so I’m not all that concerned as long as we get consensus on the correct approach.
- @clarkeemily I’d like to suggest picking this up async, perhaps on the GitHub issue? We can also add an agenda item to revisit this time next week too if no conclusion is drawn on the GH issue? We have a few more project updates to go through, so we can circle back async
Images
Contributors: @flixos90 @thekt12 @adamsilverstein @joemcgill
- @flixos90 Just a quick reminder that the Fetchpriority standalone plugin was published on Friday, the Fetchpriority standalone plugin was approved for wordpress.org. Since we already have the infrastructure for it set up in
WordPress/performance, and since we’ve already established that it works with the WebP Uploads standalone plugin, I’m going to go ahead and publish a first version now. Will share progress here
Measurement
Contributors: @adamsilverstein @olliejones @joemcgill @mukesh27
- @joemcgill I plan to open a few tickets for some enhancements we’d like to make to the automated performance tooling we added to core during the 6.2 milestone, but no immediate work happening there.
Ecosystem Tools
Contributors: @joegrainger
- @joegrainer I have been away the past week so not much to update here from me but I will be working through the issues raised in the Plugin Checker Milestone 1 Review/QA. We have made some progress and closed a few of these out already. You can follow all progress on the GitHub repo here. Thanks!
Creating Standalone Plugins
Contributors: @flixos90 @mukesh27 @10upsimon
- @flixos90 Just the above, Fetchpriority was published, and following that I submitted Dominant Color Images for plugin review (which is expected to take 25+ days, so we’ll have some time to wait)
- @mukesh27 The PR Use dynamic version from
plugins.jsonfor manual workflow #710 ready for review
Open Floor
- @thelovekesh Since the performance plugin is expanding in terms of the number of classes, functions, and hooks it contains, I believe it would be great to start documenting things so that plugin and theme developers may use them. We can start using the phpdoc-parser to automatically create the necessary documentation from code comments when a release is made and the same can be deployed to the GitHub pages site(https://wordpress.github.io/performance/) or to WordPress.org.
Our next chat will be held on Tuesday, May 16, 2023 at 15:00 UTC in the #core-performance channel in Slack.