-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Slightly overhaul performance tests #5679
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @swissspidy, I really like these updates.
Do we need to make any updates to the https://www.codevitals.run/project/wordpress dashboard to be able to collect some of these new metrics (e.g., DB calls, additional themes, etc.) over time or adjust anything else about how these are now being collected?
I also think we may want to consider breaking this workflow into two separate workflows:
the performance test runner, and the main performance test workflow that defines the matrix and uses the runner to collect results, similar to how the phpunit-tests.yml
and phpunit-tests-run.yml
workflows are set up. This would make it easier for us to run manual workflows for a specific use case. However, this is a good iteration for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @swissspidy for the PR! Nice work. Left some questions.
I updated the log script so that the existing metrics should be collected as usual. Now that we finally collect memory usage and server-timing in the admin, those fields, which were previously empty, can finally be displayed properly too. For tracking things like db queries, changes to the dashboard are indeed necessary, but I usually handle this on Slack directly with Riad :) That said, the dashboard really doesn't scale for that many metrics. So if we want to track all the default themes etc. then we need to improve/switch the dashboard first. |
I agree that the UI of the dashboard is not meant to handle a huge list of metrics. I don't mind much If we think there are alternative dashboards that we can switch to without too much hassle. If not, we can also implement this issue to have like "main metrics" and "all metrics" pages per project or something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GHA related aspects look great. I added a few questions/suggestions.
@joemcgill I'm working on something like this in #6232 for all workflows because there are huge benefits for maintainability. Let's overhaul the workflow first, and then work on splitting it into a callable workflow there. |
Committed in https://core.trac.wordpress.org/changeset/58076 🤞 |
This originally started as a proof-of-concept for running tests with object cache, but ended up covering quite a bunch more.
Example:
(View workflow run)
Specifically:
for
loops instead.log-results.js
so that the expected format stays the same, prevents the dashboard from breaking.Server-Timing
.Server-Timing
header on the admin so we get more insights there.repeatEach
in Playwright. Now there are 2 repetitions with 20 iterations each.compare-results.js
to improve output. Adds standard deviation (STD) and median absolute deviation (MAD) as well.Good to know:
wpDbQueries
is0
in the memcached results test, it means all db queries were served from the cache in subsequent tests. If the number is higher, than there are some uncached queries.Trac ticket: https://core.trac.wordpress.org/ticket/59900
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.