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

feat(tasks): shard benchmarks in CI #2751

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Mar 17, 2024

This PR shards benchmarks when running on CI. Each benchmark (parser, minifier etc) runs as a separate job, and then a final job combines the results and uploads to Codspeed.

A bit of a hacky implementation. Uses a small NodeJS HTTP server to intercept the results from codspeed-runner, and then another NodeJS script to combine them all together, and upload to CodSpeed.

I will submit PRs on Codspeed's runner + action to do it properly, but as I imagine it'll be a slow process getting that merged upstream, I wanted to see if it worked first. We can replace this once it's supported upstream.

Sharding only reduces total time to run the benchmarks by about 70 secs at present, because linter benchmark takes 6 mins alone and holds up the whole process (all the rest are done in ~2 mins). If we can split up the linter benchmark, we can likely get total run time down to around 3 mins. I'll try that in a follow-on PR.

I guess the other upside is we can now add as many benchmarks as we like with impunity - they'll run in parallel, and so won't slow things down overall.

Copy link
Contributor Author

overlookmotel commented Mar 17, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @overlookmotel and the rest of your teammates on Graphite Graphite

Copy link

codspeed-hq bot commented Mar 17, 2024

CodSpeed Performance Report

Merging #2751 will not alter performance

⚠️ No base runs were found

Falling back to comparing 03-17-feat_tasks_shard_benchmarks_in_CI (21c5dcb) with main (1e9c0bc)

Summary

✅ 29 untouched benchmarks

Copy link
Member

Boshen commented Mar 18, 2024

Merge activity

  • Mar 17, 10:45 PM EDT: @Boshen started a stack merge that includes this pull request via Graphite.
  • Mar 17, 10:45 PM EDT: @Boshen merged this pull request with Graphite.

@Boshen Boshen merged commit 93e1ea4 into main Mar 18, 2024
27 checks passed
@Boshen Boshen deleted the 03-17-feat_tasks_shard_benchmarks_in_CI branch March 18, 2024 02:45
Boshen pushed a commit that referenced this pull request Mar 18, 2024
Follow-on from #2751. Further shards linter benchmarks so each fixture runs in its own job.

This reduces total time to run benchmarks by another ~75 secs. So approx 2.5 mins shaved off in total.
overlookmotel added a commit that referenced this pull request Mar 18, 2024
#2751 contained a mistake, which was pointed out by Adrian @ CodSpeed on
Discord.

For PRs from forks, `CODSPEED_TOKEN` is not provided, and the submission
to CodSpeed is "tokenless". #2751 wrongly assumed all runs are submitted
with a token. This PR fixes that.
overlookmotel added a commit that referenced this pull request Mar 18, 2024
Improvement to #2751. CodSpeed have advised no need to upload the log
files.
@JoSeBu1
Copy link
Contributor

JoSeBu1 commented Mar 18, 2024

One thing, now when a PR is done at some times there are like 18 parallel jobs and if another PR starts while one PR is checking, the second one is waiting. (The benchmark launches 8 parallel jobs). And the jobs of one PR and the other take turns. This may cause PR checks to take longer now than before with this cause. Taking between 9 and 10 minutes each PR. I'm just writing it down out of curiosity, maybe you already knew it.

@overlookmotel
Copy link
Contributor Author

the jobs of one PR and the other take turns

Yes, this is true. Thanks for pointing it out. However, this only happens if there's 2 PRs trying to run benchmarks at same time, and most of the time this won't be the case. I must say your rate of production of PRs is unusually high!

I also hope we can further speed up the benchmarks by better build caching, which will reduce the frequency of "overlaps" further.

So, while the problem you've identified is real, I do feel it's the best trade-off available at this time.

Hope this makes sense.

@JoSeBu1
Copy link
Contributor

JoSeBu1 commented Mar 19, 2024

Just as an idea, I understand that we can ignore it for now but I was thinking about some possible solution and it occurred to me that maybe we can put some of the short-lived CI jobs sequentially instead of in parallel, since the final time would be the same and we save launching some jobs in parallel. that can be used in another pr for example.

@overlookmotel
Copy link
Contributor Author

@JoSeBu1 That's a very good idea! Let's see if we can get the benchmark build times down first, as that'd change the calculation of how many and which to run sequentially. But then, yes, let's look into doing exactly that.

@JoSeBu1
Copy link
Contributor

JoSeBu1 commented Mar 19, 2024

Ok, about the cache, this can be tricky because as I see now we are running out of space, because we have more than 10Gb in cache, maybe we should review what is cached, if it's necessary, and the TTLs.

@overlookmotel
Copy link
Contributor Author

@JoSeBu1 Just so the (very good) points you've made here don't get lost/forgotten, would you mind opening a separate issue for them?

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

Successfully merging this pull request may close these issues.

3 participants