-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
GH Actions/quicktest: use Linux Arm64 #791
Conversation
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 changes here look good to me. I think we can switch over all the PHP 5.x builds without much impact.
I see that we'll need to set COVERAGE_REPORTER_PLATFORM
so that the correct flavour of binary gets installed for the Coveralls part to work successfully. Ideally that fix gets pushed to the script itself rather than having to set the variable, as uname -m
will give the value that they're looking for. See coverallsapp/github-action#218
Edit: This change should make the tests pass here. coverallsapp/github-action#238
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.
While there are more places where this change can be applied, this is a good first step.
@fredden I was just about to look at the build failure when I got interrupted by a phone call. Thanks for finding the root cause so quickly! coverallsapp/github-action#238 looks like magic to me. 🙌🏻 I'm going to give it a few days, but may just update this PR with the |
@afinetooth That's awesome! Thank you for working with us on this. Once the release is out, I will rebase this PR to retrigger the build and 🤞🏻 |
Let me know if you have any issues! 🤙 Have a great weekend! 🎉 |
377d632
to
fa42131
Compare
Thanks @afinetooth ! I've rebased the PR without changes. Build is running. |
@fredden @afinetooth Uh-oh... looks like the build is still failing: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/12958904548/job/36150203750 @fredden Also: for the "quicktest" workflow (just test, no Xdebug or code coverage), the PHP 5.4 run is about 25% faster (~6 vs 8 minutes), but for the "coverage" job, running on arm64 unfortunately doesn't look to make any difference... If you have any bright ideas about this, I'd be happy to hear them. |
@jrfnl OK, I'll check it out... |
@afinetooth Let me know if you want me to do a run with |
@jrfnl yes, The (I assume
But right now, I can't tell for sure what the architecture of the runner was, so I would love your help adding a debug line showing what the value of Unless I'm missing it somewhere in your workflow, you are not explicitly setting A little more on As I say I assume its I can't tell from the "set up job" step---can you?:
The closest hint after that is this line in the raw logs:
But that's just the version of This should expose the "sure things" (
|
fa42131
to
909aa60
Compare
👀 🙏 |
@afinetooth I've added a debug commit which adds the "Print Runner Architecture" step as per above as the first step in the coverage runs + has This will be the job to examine once it finishes in 25 minutes or so: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/12960113668/job/36153483913
Correct as, if I understood things correctly, that should be set automagically now PR coverallsapp/github-action#238 has been merged & released. |
Same here. So if it wasn't set explicitly, that suggests We'll see in ~25 min! 🍿 |
@afinetooth I may need to bow out for some 💤 before the run finishes, but will happily make any further debug adjustment tomorrow. |
So either:
Re-examining the code, and the run... |
I've just had a look at this now. The variable has a default value set of "x86_64". To fix this, we can either change the default (to an empty string), or remove the variable altogether. @afinetooth, which option would you prefer? I'm happy to open a pull request to sort this out. Edit: I have opened coverallsapp/github-action#240 in anticipation of the "change the default" option. I'm happy to adapt this into "remove the parameter" if you would prefer. |
@fredden removing the default value of Reviewing your PR... |
@fredden @afinetooth Thank you both for working with me on this. I've retriggered the build for PHP 5.4, so let's see in 25 minutes or so 🤞🏻 |
@fredden @jrfnl |
Checks are now passing. 🎉 |
Looks like the upload was successful now! 🎉 @fredden There's still the question about how useful this is for now. Have you got an opinion ?
I would also be happy to update/expand the PR, to have a more balanced mix of Linux vs ARM64 build, though I do also want to continue to have at least some runs on "plain" Linux. |
Thank you and congratulations, you guys. 🎉 |
It does look like the performance (ie, time) benefit is minimal. Looking at a recent run with and without this change, it looks like the arm64 version actually took one minute longer. x86_64: 25m40s (unrelated merged pull request) This page has an interesting analysis of arm64 versus x86_64: https://fraudmarc.com/post/arm64-vs-x86-64-for-php While this doesn't achieve the performance boost that we had hoped for, I think that there is value with this change in that we are now testing the software on a slightly wider set of platforms. Perhaps we can change one or two other builds over to arm64 too, and then say that we run on a selection for "coverage" but not for performance. |
@fredden Thanks for sharing your thoughts.
I've read the article you linked. Test/coverage builds run with
While I can see the benefit from a "we've tested with this" point of view, from a functional testing point of view, there is no real benefit as the code base doesn't contain any code which is expected to behave differently on My current thinking considering the minimal performance benefits:
What say you ? |
Yes, that sounds like a good plan to me. 👍 |
GitHub has made Linux arm64 runners generally available and running tasks on these images instead of the traditional images can deliver up to a 40% performance boost. Based on our testing with this, the performance benefit is minimal to non-existent in our test setup and as the code base contains no `arm64` specific code/conditions, there is also no _functional_ benefit to running the tests on `arm64`. Having said that, having some test runs on `arm64` can serve as an early detection system in case code changes would be needed in the future. With this in mind and after some discussion about this, this commit ONLY introduces `arm64` in the `quicktest` workflow which is run for branch pushes (with the exception of pushes to `master`). That means that in practice, it will run: * When contributors with `push` rights push a branch directly to this server. * When contributors without `push` rights, but who have enabled workflow runs on their own fork of the repo, push a branch to their fork. The "normal" (PR) `test` runs will not use `arm64` at this time. This does mean that a test run failure against `arm64` for a PR contribution via a fork _could_ go unnoticed, but as stated above: as the code contains no `arm64` specific conditions, this is deemed unlikely and is seen as an acceptable risk. If needs be, this can be revisited in a later iteration. Refs: * https://github.blog/news-insights/product-news/arm64-on-github-actions-powering-faster-more-efficient-build-systems/ * https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources * https://fraudmarc.com/post/arm64-vs-x86-64-for-php
909aa60
to
f85a36e
Compare
Thanks for working with me on this @fredden ! I've now:
I believe this is now ready for a final review. |
Description
ℹ️ PR title and description has been updated to reflect the latest state of the PR.
GitHub has made Linux arm64 runners generally available and running tasks on these images instead of the traditional images can deliver up to a 40% performance boost.
Based on our testing with this, the performance benefit is minimal to non-existent in our test setup and as the code base contains no
arm64
specific code/conditions, there is also no functional benefit to running the tests onarm64
.Having said that, having some test runs on
arm64
can serve as an early detection system in case code changes would be needed in the future.With this in mind and after some discussion about this, this commit ONLY introduces
arm64
in thequicktest
workflow which is run for branch pushes (with the exception of pushes tomaster
).That means that in practice, it will run:
push
rights push a branch directly to this server.push
rights, but who have enabled workflow runs on their own fork of the repo, push a branch to their fork.The "normal" (PR)
test
runs will not usearm64
at this time.This does mean that a test run failure against
arm64
for a PR contribution via a fork could go unnoticed, but as stated above: as the code contains noarm64
specific conditions, this is deemed unlikely and is seen as an acceptable risk.If needs be, this can be revisited in a later iteration.
Refs:
Suggested changelog entry
N/A