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

Ci: improve install time iteration 2 #16638

Conversation

belgattitude
Copy link
Contributor

@belgattitude belgattitude commented May 6, 2023

What does it do?

Improve ci time by tackling install, part of

See #16581 and the vision: #16581 (comment)

Context

In the first iteration:

  • The safest way. I just fixed a cache and provided a custom install action. Measured two possible alternatives (only yarn cache vs yarn cache + install_state + node_modules). Based on that yarn cache only was choosen (very close to each others)

This second iteration will be more open to discussion and re-assessing the previous choices based on this new situation

Why ?

Cause in the current state it brings modification to the yarn.lock and have an impact on local disk usage.

But there's two ways I can do it

Note that while for the docker example above is fine, it's not yet on CI with some hacks cause it produce a new yarn.lock. I'll coordinate with upstream yarn. Or ... I bring a solution but it will look a bit hacky.

I'll try to document in the files for you to get a clear idea.

But I'm waiting first for iteration 1 to be merged (will be clearer to follow).

And as in the previous iteration, I'll need

  • To measure / benchmark the options I have, I'll ask for workflow approval :)

Next steps

  • If we choose to change the yarn.lock we'll have to provide a small doc/warning to all open PR and/or maintainers. Cause they'll have to refresh the lock (easy: yarn install then push the lock - it won't change versions, just checksums)
  • When we align on the solution, I'll bring a 3rd iteration that will try to extract 5-10% more speedup. But it's related to how we decide to merge this one

There's even more to do... but after those I'll have to request/coordinate with the upstream yarn codebase and if it works the cache of install-state/node_modules cache will make sense to be enabled. But that's a longer road ;) I like 🌳

Why is it needed?

🌳

@@ -1,11 +1,17 @@
nodeLinker: node-modules

enableGlobalCache: false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some might wonder, why it's suddently needed. Cause afterall it's overridden in the install action.

Context:

PS: enableGlobalCache might be default in upcoming yarn version, so we align for the future as well)

plugins:
- path: .yarn/plugins/@yarnpkg/plugin-interactive-tools.cjs
spec: '@yarnpkg/plugin-interactive-tools'

defaultSemverRangePrefix: ''

compressionLevel: 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the zip isn't compressed. More work for action/cache restore / network / decompress (zstd detects when it can compress or not so... now the *.zip files will be compressable by action/cache -> slower but more compressed too)

For the curious:

My previous benchmarks +/- based on https://github.com/belgattitude/nextjs-monorepo-example

Speed

CI Scenario Install CI fetch cache CI persist cache Setup
yarn4 mixed-compression ±42s ±3s (±9s) 0s
yarn4 no compression ±34s ±4s (±6s) 0s
pnpm8 ±19s ±8s (±29s) 2s

Cache budget

CI Scenario Fetch cache Persist cache Size
yarn4 mixed-compression ±5s ±6s 220MB
yarn4 no compression ±8s ±9s 180MB
pnpm7 ±14s ±16s 273MB

More metrics in https://github.com/belgattitude/compare-package-managers

@@ -1,11 +1,17 @@
nodeLinker: node-modules

enableGlobalCache: false

nmMode: hardlinks-local
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not totally required. We'll see the ultimate choice made by upcoming yarn 4. But I'd say... keep it there explicitly (in yarn 3 it's the default).

I'm open to remove this too (anyway the install action explcitly overrides it to 'hardlinks-local')

@gu-stav
Copy link
Contributor

gu-stav commented May 10, 2023

@belgattitude I've merged your other PR - could you update this one with main? 🙏🏼

@belgattitude
Copy link
Contributor Author

belgattitude commented May 10, 2023

@gu-stav done, but I'll have to double check how I've impacted the lock during the merge. So consider this more a wip till I'm sure nothing have been silently updated. BTW github action seems down... so let's move slowly

@belgattitude
Copy link
Contributor Author

belgattitude commented May 11, 2023

@gu-stav I keep this note as a reminder (about something unrelated to this P/R)

one thing that I realized also is that the deps vscode/sqlite3 is consuming time and does not seem to complie in CI anyway. I'll do a cost/benefit P/R later on. Not sure of the value in the getting-started example

yarn why @vscode/sqlite3
└─ getstarted@workspace:examples/getstarted
   └─ @vscode/sqlite3@npm:5.1.2 (via npm:5.1.2)

See the full run https://github.com/strapi/strapi/actions/runs/4946607251/jobs/8845313813

image

Impact is important and does not work very well in ci's (the build will fail)

image

Myself I would remove this dep in the example. Value is limited if you're installing better-sqlite3 in the example

https://github.com/strapi/strapi/blob/main/packages/core/database/lib/connection.js#L38

  // NOTE: this tries to find the best sqlite module possible to use
  // while keeping retro compatibility
  return (
    trySqlitePackage('better-sqlite3') ||
    trySqlitePackage('@vscode/sqlite3') ||
    trySqlitePackage('sqlite3')
  );

Or keep the examples outide of the main workspace (even better). Many options possibles.

The suggested P/R is there #16693

@belgattitude
Copy link
Contributor Author

Would be awesome to have something labelling the status to run the full test suite ❤️

@belgattitude
Copy link
Contributor Author

Next iterations will be

But I'm confident that we can reach a full test on < 45 minutes (vs +/-1h30 before iteration 1) soon. And more ideas to come.

@innerdvations innerdvations added pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: tooling Source is GitHub tooling/tests/ect labels May 15, 2023
@innerdvations innerdvations added this to the 4.11.0 milestone May 15, 2023
@belgattitude
Copy link
Contributor Author

belgattitude commented May 15, 2023

Got the measurements now.

setting the compressionLevel to 0 only saves 5-20s per install. With CE there's around 10 steps. (the saving is 1min - 3minutes only per push/run)

image

So I'm not sure. To keep it short,

Trade-offs:

  • For dev local install zip files aren't compressed: 1gb on disk vs 250Mb
  • For CI only +/-2 minutes saving per run (tests/ce).
  • For CI as the zip appears now to be compressible: saves 100Mb on cache budget (loose speed on restore as action/cache/zstd strats to compress the zips. so the overhead is moved to decompression).
  • If using docker at some point, compressionLevel:0 keep more optims possibles.

So it all depends... Wdyt ?

For me this PR can be merged safely... and in my experience compressionLevel:0 is a good thing, cause more deps you add, more gain you get (till this lands nodejs/node#45651 but it's a long way).

PS: next iterations: #16694 and/or #16721 are part of the serie too.

@gu-stav
Copy link
Contributor

gu-stav commented May 15, 2023

@belgattitude it seems possible to use env variables in the yarnrc file. Could we set the compression level only in a CI environment?

yarnpkg/berry#1341

@belgattitude
Copy link
Contributor Author

belgattitude commented May 15, 2023

Not really... as the lock contains checksums. Whenever you change compression they are recalculated.

It's possible to hack of course... That's actually how I speed up docker images build:

RUN YARN_ENABLE_GLOBAL_CACHE="false" \
    YARN_CACHE_FOLDER=/var/cache/yarn-artist-strapi \
    YARN_COMPRESSION_LEVEL=0 \
    YARN_CHECKSUM_BEHAVIOR=ignore \
    yarn install --immutable --inline-builds

but I don't recommend cause right now it changes the yarn.lock as well. so you'll need to rollback the change after install. for docker it's fine (divide install by 2)

But that's something I'll propose to change or clarify in upstream yarn. Will PR next week and see how it's received (cause I'm not sure)

@gu-stav
Copy link
Contributor

gu-stav commented May 15, 2023

@belgattitude thanks. I'll do some local testing tomorrow, but I think the size difference wouldn't be a huge problem.

@joshuaellis @Convly @innerdvations what do you think?

@innerdvations
Copy link
Contributor

Compression level I'm not so sure about. It seems like a fairly minimal improvement compared to the tradeoffs. Are the next iterations dependent on it or can it be left alone for now?

@belgattitude
Copy link
Contributor Author

belgattitude commented May 16, 2023

@innerdvations seems okay to me. I'll move it to the later iteration and when I can measure the impact based on final round about installs.

So I propose this order

Then I'll measure the new baseline and adapt

That's the easiest way for me

Thanks

@Convly Convly modified the milestones: 4.11.0, 4.11.1 Jun 7, 2023
@Marc-Roig Marc-Roig modified the milestones: 4.11.1, 4.11.2 Jun 12, 2023
@Convly Convly modified the milestones: 4.11.2, 4.11.3 Jun 21, 2023
@Marc-Roig Marc-Roig modified the milestones: 4.11.3, 4.11.4 Jun 26, 2023
@Convly Convly modified the milestones: 4.11.4, 4.11.5 Jul 5, 2023
@Marc-Roig Marc-Roig modified the milestones: 4.11.5, 4.11.6 Jul 10, 2023
@gu-stav gu-stav removed this from the 4.11.6 milestone Jul 17, 2023
@gu-stav gu-stav closed this Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: tooling Source is GitHub tooling/tests/ect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants