.github/workflows/push.yml: enable ccache by MaxKellermann · Pull Request #10395 · php/php-src

MaxKellermann · 2023-01-21T08:25:55Z

This reduces the LINUX_X64_RELEASE_ZTS build time from 9-10 minutes to less than 3 minutes.

mvorisek · 2023-01-21T10:42:01Z

can a job name be used here? if not, you can use https://docs.github.com/en/actions/learn-github-actions/expressions#tojson on whole matrix

also the cache key should contain the minor PHP version/branch, you can use hash of https://github.com/php/php-src/blob/master/main/php_version.h file - https://docs.github.com/en/actions/learn-github-actions/expressions#hashfiles

Will do! (This evening or so.)

I wonder if this should contain the branch name? This is targetting master but it would make sense to backport. In that case the cache key should be different to avoid invalidating the cache of other branches.

2x yes - master will be branch cutted someday and PHP-8.1/8.2 CI will benefit from this PR as well

devnexen · 2023-01-21T10:51:36Z

this is a nice addition

iluuu1994

Awesome! Thought about this just yesterday 🙂

Can we add the same for the nightly builds?

mvorisek · 2023-01-21T13:34:21Z

Can we add the same for the nightly builds?

I would prefer non-cached build in nightly. A few more minutes in nightly are not critical and it represents a standard/clean build.

iluuu1994 · 2023-01-21T13:35:50Z

@mvorisek Also burns lots of CI time, given how many jobs there are. If we have problems we can always remove it. Sadly, barely anybody looks at nightly anyway. And tests are so flaky there are few days when I don't wake up with an e-mail.

TimWolla · 2023-01-21T13:43:20Z

And tests are so flaky

This is why I am also a little concerned about introducing caching in CI: It is another possible failure mode. Is the build step sufficiently stable that caching won't introduce another failure mode, because some file wasn't rebuilt despite some dependency changing?

iluuu1994 · 2023-01-21T13:49:32Z

Using ccache locally, I've never had caching issues due to it. The only time I need to do a full rebuild is if autoconf stuff changes, but I still never have to manually clear ccache. IMO this is certainly worth a try. If we encounter issues we can always revert it.

iluuu1994 · 2023-01-21T13:58:19Z

It looks like Travis already uses ccache. While not being exactly the most stable, cache never seemed to be the cause for that.

MaxKellermann · 2023-01-21T14:36:39Z

Is the build step sufficiently stable that caching won't introduce another failure mode, because some file wasn't rebuilt despite some dependency changing?

Agree with @iluuu1994 - I've been using ccache for many years, and it was never the cause of a problem. It's very mature and saved me so much lifetime.

Now if we could only speed up all those unit tests... the C compiler is only a small part of the CI run.

MaxKellermann · 2023-01-21T16:56:20Z

Implemented two suggestions by @mvorisek:

job name is now in the cache key; unfortunately, this required some "code" duplication (see comment in the diff)
hash of main/php_version.h added to cache key so each branch gets its own cache

iluuu1994

hashFiles('main/php_version.h') is a nice approach, thanks! Would you like to add nightly here or in a separate PR? I'm happy either way. Could you rebase this onto PHP-8.1?

MaxKellermann · 2023-01-21T20:18:10Z

Technically you could still use ${{github.job}} instead of duplicating LINUX_X64 above, right? Not a big deal, just for consistency.

True, but since it wasn't used for the job name, I didn't use it here either.

mvorisek · 2023-01-22T09:40:20Z

Suggested change

# This duplicates the "job.name" expression above because

# GitHub has no way to query the job name (github.job is the

# job id, not the job name)

key: "LINUX_X64_${{ matrix.debug && 'DEBUG' || 'RELEASE' }}_${{ matrix.zts && 'ZTS' || 'NTS' }}-${{hashFiles('main/php_version.h')}}"

key: "${{ github.workflow }}-${{ github.job }}-${{ toJSON(matrix) }}-${{ hashFiles('main/php_version.h') }}"

That's better because it doesn't duplicate the expression, but it's worse because a hash is more obscure. I don't have a real opinion on what to choose.

no hash added and json is quite readable :)

Ah, it's JSON, not hash of a JSON ... yes, that makes sense.

Doesn't work:

Error: Restoring cache failed: ValidationError: Key Validation Error: ccache-Push-LINUX_X64-{ "debug": true, "zts": false }-10c280ee69e81a8f532bab2756a061cccc70829ec83708c23eb95d1f14f49c53- cannot contain commas.

Hash of JSON would solve that, but no hash function exists :( :) What about moving the ccache step into separate file and calculating the cache key there using bash?

see https://github.com/atk4/core/blob/develop/.github/workflows/test-unit.yml l37 and l42 for how data can be passed between steps, ie. from bash step to GH action step

I don't think that's worth the complexity.

mvorisek · 2023-01-22T09:42:36Z

This reduces the LINUX_X64_RELEASE_ZTS build time from 9-10 minutes to less than 3 minutes.

Any idea where the compiler spents the rest of the time and if something can be further cached or improved?

MaxKellermann · 2023-01-22T11:09:38Z

Any idea where the compiler spents the rest of the time and if something can be further cached or improved?

I/O and the linker. The linker output can't be cached. Switching to mold will give a big performance boost here. mold however isn't available in Ubuntu 20.04; 22.04 contains an old version of mold.

iluuu1994 · 2023-01-22T11:41:07Z

I'd be ok with switching to mold for some of our builds but we should definitely have at least one full nightly build that uses ld given that's most likely what the majority of builds use out there.

This reduces the LINUX_X64_RELEASE_ZTS build time from 9-10 minutes to less than 3 minutes.

MaxKellermann · 2023-01-22T18:40:18Z

I'd be ok with switching to mold for some of our builds but we should definitely have at least one full nightly build that uses ld

Agree. I'll submit a PR to upgrade push.yml to Ubuntu 22 and switch to mold. nightly.yml remains unchanged, so possible linker problems (which I assume will be extremely rare, if ever) get detected daily/nightly.

iluuu1994 · 2023-01-22T18:47:25Z

@MaxKellermann Let's merge this like this, Ubuntu/mold can be done in a separate PR. You don't have to rebase, I'll do that when I merge this.

MaxKellermann · 2023-01-22T18:50:03Z

in a separate PR

New PR, I meant exactly that.

iluuu1994 · 2023-02-02T18:00:39Z

Thanks again! And sorry for the delay.

mvorisek · 2023-02-03T00:54:14Z

in the CI there are warnings like: https://github.com/php/php-src/actions/runs/4077782444/jobs/7027216607#step:8:9

LINUX_X32_DEBUG_ZTS takes ~4 minutes, can the build times of x64/NTS be reduced, why they take 2x longer?

MaxKellermann · 2023-02-03T09:29:19Z

in the CI there are warnings like: https://github.com/php/php-src/actions/runs/4077782444/jobs/7027216607#step:8:9

This has nothing to do with ccache; maybe this is just now more likely to happen now that the compiler is faster. Less percentage of the time spent in the compiler, more spent in administrative stuff like making directories, and now conflicts are more likely to happen.

LINUX_X32_DEBUG_ZTS takes ~4 minutes, can the build times of x64/NTS be reduced, why they take 2x longer?

Build of LINUX_X32_DEBUG_ZTS took 1m51s (95% ccache hits), and LINUX_X64_DEBUG_NTS took 2m46s (91% ccache hits). Both are pretty good values, but this depends on the ccache hit rate and of course the performance of CI VMs varies a lot; at these low build times, the noise is very high, and not much can be deduced from the cached build times.

As I said, mold would help a lot, but I didn't get around to experiment with it yet.

iluuu1994 · 2023-02-06T23:05:45Z

This seems to use too much cache storage.

https://github.com/php/php-src/actions/caches

 ccache-LINUX_X64_DEBUG_NTS-10c280ee69e81a8f532bab2756a061cccc70829ec83708c23eb95d1f14f49c53-2023-02-06T22:43:16.415Z

It seems like we need to set append-timestamp to false. See hendrikmuhs/ccache-action#126.

We also seem to have some caching issues, e.g. here.

MaxKellermann · 2023-02-23T10:22:44Z

btw. here's a fix that finally allows using mold: #10678

MaxKellermann force-pushed the ccache branch 2 times, most recently from 8c01d4e to 3915f92 Compare January 21, 2023 10:32

MaxKellermann marked this pull request as ready for review January 21, 2023 10:32

mvorisek reviewed Jan 21, 2023

View reviewed changes

TimWolla requested a review from iluuu1994 January 21, 2023 13:14

iluuu1994 reviewed Jan 21, 2023

View reviewed changes

MaxKellermann force-pushed the ccache branch from 3915f92 to a4323c5 Compare January 21, 2023 16:54

MaxKellermann mentioned this pull request Jan 21, 2023

.github/workflows/push.yml: add job to build out-of-tree extensions #10404

Closed

TimWolla reviewed Jan 21, 2023

View reviewed changes

Comment thread .github/workflows/push.yml Resolved

iluuu1994 approved these changes Jan 21, 2023

View reviewed changes

Comment thread .github/workflows/push.yml Resolved

mvorisek reviewed Jan 22, 2023

View reviewed changes

MaxKellermann force-pushed the ccache branch from a4323c5 to 7497bdf Compare January 22, 2023 13:10

.github/workflows/push.yml: enable ccache

506b3d1

This reduces the LINUX_X64_RELEASE_ZTS build time from 9-10 minutes to less than 3 minutes.

MaxKellermann force-pushed the ccache branch from 7497bdf to 506b3d1 Compare January 22, 2023 13:15

iluuu1994 closed this in f7e6784 Feb 2, 2023

MaxKellermann deleted the ccache branch February 3, 2023 09:30

-          # This duplicates the "job.name" expression above because
-          # GitHub has no way to query the job name (github.job is the
-          # job id, not the job name)
-          key: "LINUX_X64_${{ matrix.debug && 'DEBUG' || 'RELEASE' }}_${{ matrix.zts && 'ZTS' || 'NTS' }}-${{hashFiles('main/php_version.h')}}"
+          key: "${{ github.workflow }}-${{ github.job }}-${{ toJSON(matrix) }}-${{ hashFiles('main/php_version.h') }}"

Conversation

MaxKellermann commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvorisek Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devnexen commented Jan 21, 2023

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

mvorisek commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluuu1994 commented Jan 21, 2023

Uh oh!

TimWolla commented Jan 21, 2023

Uh oh!

iluuu1994 commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluuu1994 commented Jan 21, 2023

Uh oh!

MaxKellermann commented Jan 21, 2023

Uh oh!

MaxKellermann commented Jan 21, 2023

Uh oh!

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxKellermann commented Jan 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvorisek commented Jan 22, 2023

Uh oh!

MaxKellermann commented Jan 22, 2023

Uh oh!

iluuu1994 commented Jan 22, 2023

Uh oh!

MaxKellermann commented Jan 22, 2023

Uh oh!

iluuu1994 commented Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxKellermann commented Jan 22, 2023

Uh oh!

iluuu1994 commented Feb 2, 2023

Uh oh!

mvorisek commented Feb 3, 2023

Uh oh!

MaxKellermann commented Feb 3, 2023

Uh oh!

iluuu1994 commented Feb 6, 2023

Uh oh!

MaxKellermann commented Feb 23, 2023

Uh oh!

Reviewers

MaxKellermann commented Jan 21, 2023 •

edited

Loading

mvorisek Jan 21, 2023 •

edited

Loading

mvorisek commented Jan 21, 2023 •

edited

Loading

iluuu1994 commented Jan 21, 2023 •

edited

Loading

iluuu1994 commented Jan 22, 2023 •

edited

Loading