Skip to content
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: speed up slow jobs by using Linux Arm64 #791

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jan 23, 2025

Description

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.

As the fast majority of end-users won't be using Arm yet, I'm not (yet) switching all Linux builds to Arm runners, but for now, I think it makes sense to try and speed up the build by using these images for the slowest builds (PHP 5.4).

Refs:

Suggested changelog entry

N/A

@jrfnl jrfnl added this to the 3.12.0 milestone Jan 23, 2025
@jrfnl jrfnl requested a review from fredden January 23, 2025 20:24
Copy link
Member

@fredden fredden left a 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

Copy link
Member

@fredden fredden left a 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.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 23, 2025

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

@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've also opened a loosely related PR - coverallsapp/github-action#239 , though that one may not be needed once coverallsapp/github-action#238 gets merged.

I'm going to give it a few days, but may just update this PR with the coverage-reporter-platform input to make it mergeable. We can always remove it again later once the upstream PR(s) have been merged.

@afinetooth
Copy link

@fredden @jrfnl Thanks for your PRs. I've merged them both and am cutting new release of coverallsapp/github-action@v2 right now. Should be v2.3.5 but @v2 will continue pointing to the latest release.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 24, 2025

@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 🤞🏻

@afinetooth
Copy link

afinetooth commented Jan 24, 2025

@jrfnl (Cc: @fredden)

Let me know if you have any issues! 🤙

Have a great weekend! 🎉

@jrfnl jrfnl force-pushed the feature/ghactions-use-linux-arm-for-slow-builds branch from 377d632 to fa42131 Compare January 24, 2025 22:49
@jrfnl
Copy link
Member Author

jrfnl commented Jan 24, 2025

Thanks @afinetooth ! I've rebased the PR without changes. Build is running.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 24, 2025

@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.

@afinetooth
Copy link

@jrfnl OK, I'll check it out...

@jrfnl
Copy link
Member Author

jrfnl commented Jan 24, 2025

@afinetooth Let me know if you want me to do a run with debug turned on or anything.

@jrfnl jrfnl marked this pull request as draft January 24, 2025 23:56
@afinetooth
Copy link

afinetooth commented Jan 25, 2025

@jrfnl yes, debug: true would probably be helpful.

The Exec format error error indicates an architecture mismatch between the binary called and the runner.

(I assume arm64 for the runner and x86_64 for the binary. In fact, I can better than assume for the binary since the value is shown on Line 129: COVERAGE_REPORTER_PLATFORM: x86_64.)

debug: true should clarify which binary was actually called, though.

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 (uname -m) was. Or else what GitHub Actions exposes as RUNNER_ARCH, or both, which should give us that as a certainty.

Unless I'm missing it somewhere in your workflow, you are not explicitly setting coverage-reporter-platform, yet it is somehow getting set to x86_64 by virtue of COVERAGE_REPORTER_PLATFORM: x86_64 on Line 129. 🤷‍♂️


A little more on RUNNER_ARCH:

As I say I assume its arm64 from the flag-name chosen, os-ubuntu-24.04-arm-php-5.4-custom-ini-false, though that doesn't necessarily guarantee it.

I can't tell from the "set up job" step---can you?:

  • Included Software is close to certainty, but it's not the same as RUNNER_ARCH:
Screenshot 2025-01-24 at 4 36 01 PM

The closest hint after that is this line in the raw logs:

2025-01-24T22:50:24.6442845Z Acquiring 20.18.2 - arm64 from https://github.com/actions/node-versions/releases/download/20.18.2-12900461873/node-20.18.2-linux-arm64.tar.gz

But that's just the version of node being downloaded for arm64 architecture, right?


This should expose the "sure things" (RUNNER_ARCH and (uname -m):

jobs:
  check-runner-arch:
    runs-on: ubuntu-latest  # Change to your actual runner OS
    steps:
      - name: Print Runner Architecture
        run: |
          echo "RUNNER_ARCH: $RUNNER_ARCH"
          echo "uname -m: $(uname -m)"

jrfnl added 2 commits January 25, 2025 01:44
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.

As the fast majority of end-users won't be using Arm yet, I'm not (yet) switching all Linux builds to Arm runners, but for now, I think it makes sense to try and speed up the build by using these images for the slowest builds (PHP 5.4).

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
@jrfnl jrfnl force-pushed the feature/ghactions-use-linux-arm-for-slow-builds branch from fa42131 to 909aa60 Compare January 25, 2025 00:46
@afinetooth
Copy link

afinetooth commented Jan 25, 2025

👀 🙏

@jrfnl
Copy link
Member Author

jrfnl commented Jan 25, 2025

@jrfnl yes, debug: true would probably be helpful.

@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 debug: true for the coverallsapp/github-action steps.

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

Unless I'm missing it somewhere in your workflow, you are not explicitly setting coverage-reporter-platform, yet it is somehow getting set to x86_64 by virtue of COVERAGE_REPORTER_PLATFORM: x86_64 on Line 129. 🤷‍♂️

Correct as, if I understood things correctly, that should be set automagically now PR coverallsapp/github-action#238 has been merged & released.

@afinetooth
Copy link

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 (uname -m) returned x86_64---despite all other indications.

We'll see in ~25 min! 🍿

@jrfnl
Copy link
Member Author

jrfnl commented Jan 25, 2025

@afinetooth I may need to bow out for some 💤 before the run finishes, but will happily make any further debug adjustment tomorrow.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 25, 2025

image

@afinetooth
Copy link

So either:

  1. Our new code isn't working; OR
  2. COVERAGE_REPORTER_PLATFORM is getting set, to x86_64, somehow, ahead of the run.

Re-examining the code, and the run...

@fredden
Copy link
Member

fredden commented Jan 25, 2025

So either:

  1. Our new code isn't working; OR

  2. COVERAGE_REPORTER_PLATFORM is getting set, to x86_64, somehow, ahead of the run.

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.

https://github.com/coverallsapp/github-action/blob/773b6d8e80fa7862da56a7664bd747c91255b2e2/action.yml#L79

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.

@afinetooth
Copy link

afinetooth commented Jan 25, 2025

@fredden removing the default value of x86_64 for the coverage-reporter-platform option seems a good approach, since it retains the ability to force-set the value.

Reviewing your PR...

@jrfnl
Copy link
Member Author

jrfnl commented Jan 25, 2025

@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 🤞🏻

@afinetooth
Copy link

afinetooth commented Jan 25, 2025

@fredden @jrfnl v2.3.6 of coverallsapp/github-action is now used by coverallsapp/github-action@v2. 🚀 🍿 🤞

@fredden
Copy link
Member

fredden commented Jan 25, 2025

Checks are now passing. 🎉

@jrfnl
Copy link
Member Author

jrfnl commented Jan 25, 2025

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 ?

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.

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.

@afinetooth
Copy link

Thank you and congratulations, you guys. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants