Performance Chat Summary: 16 May 2023
Meeting agenda here and the full chat log is available beginning here on Slack.
Announcements
- Yesterday saw version 2.3.0 of the Performance Lab plugin released
Priority Projects
Server Response Time
Contributors: @joemcgill @spacedmonkey @aristath
- @aristath Autoloading PHP classes in WP: I’ve been doing some tests and it looks like this PR improves server response times a bit. More importantly though, it reduces memory usage so that’s a big win. The only “controversial” thing about that PR is that it allows users to override Core classes if they load their own classes with the same name early enough.
- Pros: No more user hacks in Core files! If users want to override a class in Core, it will be possible. There are many valid reasons and scenarios to do that, since not everything in Core is filterable. What that means is that users will no longer resort to editing Core files directly to do what they need to do – and therefore they will be able to update their sites normally. Responsibility to maintain their own personal overrides falls to them. Maintain your own hacks.
- Cons: It’s going to be a tough sell.
- @joemcgill Amazing work to see. Would be useful to know what support or feedback @aristath wants from us to keep this moving forward.
- @spacedmonkey just created this ticket #58327
- @spacedmonkey PRs in need of review
- @joemcgill For our goal, Identifying and addressing the largest server response time bottlenecks in WordPress core – I’ve drafted a blog post summarizing the performance analysis I did of WP 6.2 and plan to post it this week after getting a few eyes on it. @spacedmonkey who has been an early reviewer and contributor to this work has already been busy spinning up tickets for some of what was discovered.
- @spacedmonkey The PRs listed above are the “flow hanging fruit” from the review
Database Optimization
Contributors: @aristath @spacedmonkey @olliejones @rjasdfiii
- @spacedmonkey Committed
- https://github.com/WordPress/wordpress-develop/commit/1a5b52a17e0bfa1ca2d9133466b5ead098e3a1ba
- https://github.com/WordPress/wordpress-develop/commit/3114eda235d6740f82fc9f2b4bdf0735e43560ed
- https://github.com/WordPress/wordpress-develop/commit/4313210c8232cd7961f9da9660636623319070e8
- These are the final commits for my project to lazy load as much meta data as possible. Term, comment and site meta now always lazy load.
JavaScript & CSS
Contributors: @mukesh27 @10upsimon @adamsilverstein
- @joemcgill We’re continuing to track and respond to feedback on the Trac ticket for adding support for script loading strategies to core. I’ve been focused on trying to get consensus with @azaozz about the open question about how to best support
async, and in the meantime, @10upsimon has been reviewing and responding to other feedback, @10upsimon anything to add? - @10upsimon I am addressing lower hanging fruit regarding test cases, adding
@coversto tests, and adding failure messages. Regarding@coversit is generally unclear which methods and functions I should be adding, as even though we may test something as simple as an inline script in the after position attached to a deferred script, there are many functions in the logic chain that are affected, such as WP_Scripts::do_item, WP_Scripts::get_eligible_loading_strategy etc. Do we add all, some, mose affected etc? @spacedmonkey this is probably more a discussion with you.- @joemcgill Right. I know the WP Core Handbook now has a recommendation to include
@covers, but I’m not aware of any specific best practices that have been documented, so some discussion here would be helpful. - @westonruter AFAIK the gist of it is if a given test touches a given function/method, you can add
@coversto explicitly state those are being tested. (Not clear to me why this can’t be automatically detected though) - @spacedmonkey Covers is to note what the test covers. What method or function are you testing in this test.
- @10upsimon So it should include no methods or functions not explicitly visible in the test function?
- @joemcgill Official PHPUnit docs is here: https://docs.phpunit.de/en/8.5/annotations.html#appendixes-annotations-covers-tables-annotations
- Note that the docs explicitly state: This will make sure that code is only marked as covered if there are dedicated tests for it, but not if it used indirectly by the tests for a different class, thus avoiding false positives for code coverage
- @spacedmonkey Every new method or function should have a test covering it. That is the point of unit tests, to see what is / isnt covered.
- @10upsimon Sure, and they do, but we have some tests that are so reliant on – for example – other methods in the chain that are critical to the outcome of the test, but not called directly in the test. A good example is WP_Scripts::do_item and WP_Scripts::get_eligible_loading_strategy, but given that these have dedicated tests, it appears as though adding them via
@coversis not needed at all, only against tests solely created for their coverage. - @spacedmonkey You can have multiple covers comments if you want. It is common to do this in core, as lots of wrapper functions call an class. It makes sense to mark the class / method and function as covers.
- @10upsimon ^^ This is where the lines get blurred to me, this is what I have done in a recent PR that I will link to from your PR feedback, I’m just not sure where the buck should stop. Let’s pick this up in the PR itself.
- @joemcgill Right. I know the WP Core Handbook now has a recommendation to include
- @10upsimon I’m addressing @westonruter CSP feedback as next priority, which will also result in changes to the test suite
Images
Contributors: @flixos90 @thekt12 @adamsilverstein @joemcgill
- @clarkeemily work has been progressing on ‘More accurate lazy-loading’
- @joemcgill The following PRs have been opened related to and could use eyes:
- @flixos90 Yes, I’m planning to commit https://core.trac.wordpress.org/ticket/58213 (the above PR 4428) today. Any feedback on the other PRs is much appreciated. I’ll also follow up on the latest feedback that came in in the past couple hours
Measurement
Contributors: @adamsilverstein @olliejones @joemcgill @mukesh27
- No updates this week
Ecosystem Tools
Contributors: @joegrainger
- @mukesh27 I am currently working on some of the issues for Plugin Checker, and there are some PRs ready for review at https://github.com/10up/plugin-check/pulls/mukeshpanchal27, If anyone has capacity please review it. Thanks!
- @joegrainer We are working on some of the final issues for the Plugin Checker that came out of the Review/QA. This brings in some bug fixes and other enhancements as we work towards an initial release. You can follow the progress on the GitHub repo here. Thanks!
Creating Standalone Plugins
Contributors: @flixos90 @mukesh27 @10upsimon
- No updates this week
Open Floor
- @spacedmonkey This discussion has been very active – https://github.com/WordPress/performance/issues/403 Interested parties should take a look
Our next chat will be held on Tuesday, May 23, 2023 at 15:00 UTC in the #core-performance channel in Slack.