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

Use shared benchmark CI #105

Merged
merged 9 commits into from
Jan 18, 2025
Merged

Use shared benchmark CI #105

merged 9 commits into from
Jan 18, 2025

Conversation

MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Jan 15, 2025

No description provided.

@MahdiBM MahdiBM requested review from 0xTim and gwynne as code owners January 15, 2025 22:17
Copy link

github-actions bot commented Jan 16, 2025

Benchmark Report

✅ Pull request has no significant performance differences ✅

Click to expand comparison result

Benchmark check running at 2025-01-18 08:03:54 UTC

The baseline '105/merge' is EQUAL to the defined thresholds.

Click to expand benchmark result

Baseline '105/merge'

Host '1d020e19b26c' with 2 'aarch64' processors with 3 GB memory, running:
#23-Ubuntu SMP Mon Dec  9 23:51:16 UTC 2024

Parser

CollatingParserAllocations_256MiB

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) (K) * 82 82 82 82 82 82 82 1

CollatingParserAllocations_Empty

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 8 8 8 8 8 8 8 1

CollatingParserCPUTime_256MiB

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Time (user CPU) (ms) * 540 560 560 570 570 580 580 20

StreamingParserAllocations_256MiB

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) (K) * 82 82 82 82 82 82 82 1

StreamingParserAllocations_Empty

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 6 6 6 6 6 6 6 1

StreamingParserCPUTime_256MiB

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Time (user CPU) (ms) * 520 530 530 530 530 560 560 20

Serializer

100xSerializerCPUTime_1024Parts

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Time (user CPU) (ms) * 210 220 220 220 220 220 220 20

SerializerAllocations_1024Parts

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 3087 3087 3087 3087 3087 3087 3087 1

SerializerAllocations_Empty

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Malloc (total) * 4 4 4 4 4 4 4 1

Comment on lines +4 to +5
push:
branches: [main]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this. This helps with caching, as well as keeping a history of the benchmarks.

caches have branch-rules. meaning you can only retrieve a cache if it was saved by the current branch, OR the primary branch of the repo.
So the first run of a benchmark event in CI won't have a cache if we don't run benchmarks on push to main.

Though we could workaround this even without push to main. For example manually running the workflow on the main branch so it saves a base cache.

@MahdiBM MahdiBM force-pushed the mmbm-shared-benchmark-ci branch from bfdb505 to 1c51ecd Compare January 16, 2025 13:26
benchmark:
# If this is a pull-request related event, make sure this is a pr-approval event or someone has rerun the job.
if: (startsWith(github.event_name, 'pull_request') != true) || (github.event.review.state == 'approved' || github.run_attempt > 1)
uses: vapor/ci/.github/workflows/run-benchmark.yml@mmbm-benchmark
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to change @mmbm-benchmark to @main before merge

@MahdiBM MahdiBM force-pushed the mmbm-shared-benchmark-ci branch from b009c93 to 31a839f Compare January 16, 2025 13:29
@MahdiBM MahdiBM force-pushed the mmbm-shared-benchmark-ci branch from a7f07c7 to fa0919f Compare January 16, 2025 15:59
echo "Previous exit code was: $EXIT_CODE"
exit $EXIT_CODE
benchmark:
if: github.run_attempt > 1 || github.event.review.state == 'approved' || startsWith(github.event_name, 'pull_request') != true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keeping the if condition here ... should i move it to the reusable CI?

Copy link
Member

Choose a reason for hiding this comment

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

No this should probably be in the reusable CI

Copy link
Member

Choose a reason for hiding this comment

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

Disagree - different repos may not want to use the same conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm hard decision. We generally don't add the if condition to the reusable CI so I guess we can keep at that.
I can see it coming back to bite us in either case.

@MahdiBM MahdiBM changed the title Try using shared benchmark CI Use shared benchmark CI Jan 18, 2025
@gwynne gwynne merged commit 03323f0 into main Jan 18, 2025
14 checks passed
@gwynne gwynne deleted the mmbm-shared-benchmark-ci branch January 18, 2025 08:04
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.

4 participants