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

Add benchmarks #103

Merged
merged 81 commits into from
Jan 14, 2025
Merged

Add benchmarks #103

merged 81 commits into from
Jan 14, 2025

Conversation

ptoffy
Copy link
Member

@ptoffy ptoffy commented Jan 9, 2025

No description provided.

@ptoffy ptoffy requested review from 0xTim and gwynne as code owners January 9, 2025 08:49
@ptoffy ptoffy changed the title Start adding benchmarks Add benchmarks Jan 9, 2025
@MahdiBM
Copy link
Contributor

MahdiBM commented Jan 9, 2025

IMO we should use static thresholds, like SwiftNIO does.

package-benchmark relatively recently added a thresholds commands for this as well, and there is a bit of docs for it here.

I think using something like this should work to compare static thresholds against current branch:

            swift package -c release --disable-sandbox \
              benchmark thresholds check \
              --path $PWD/Benchmarks/Thresholds/ \
              --format markdown

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
Benchmarks/Sources/Benchmarks.swift Outdated Show resolved Hide resolved
@ptoffy
Copy link
Member Author

ptoffy commented Jan 9, 2025

IMO we should use static thresholds, like SwiftNIO does.

The problem with this is we don't have any baseline thresholds (yet)

@vapor vapor deleted a comment from github-actions bot Jan 9, 2025
@vapor vapor deleted a comment from github-actions bot Jan 9, 2025
@vapor vapor deleted a comment from github-actions bot Jan 9, 2025
@vapor vapor deleted a comment from github-actions bot Jan 9, 2025
Copy link

github-actions bot commented Jan 10, 2025

<See the new comment below>

/// the base sequence when an iterator is created. So you can only iterate once.
///
/// Not safe. Only for testing purposes.
/// Use `swift-algorithms`'s `AsyncSyncSequence`` instead if you're looking for something like this.
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not release the reference when an iterator is created, so it triggers CoW.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean i know why it doesn't, and it shouldn't, but that's not what we need. We want the benchmarks to be self-contained and only have minimum required overhead. If someone is using some kind of sequence with Multipart-Kit that is inefficient, then that's not our fault.

Copy link
Member

Choose a reason for hiding this comment

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

Not right now but I'm surprised there aren't existing actions for commenting on issues and we should look at turning this into its own action (or submitting it to the swift lang repo)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually there are and we were using it but someone decided to do it by hand 😆 vapor/jwt-kit#222 (comment)

Copy link
Contributor

@MahdiBM MahdiBM Jan 14, 2025

Choose a reason for hiding this comment

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

The github-scripts doesn't really help, we'll end up with around the same amount of lines of code, just in js instead of bash.
Most of the code here is to keep track of the old comment and don't create new comments.
I think Tim is talking about a dedicated action that takes some parameters and just does the work. Something like actions/checkout@v4 but for this purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we had that before, that's what I meant with my comment in JWTKit https://github.com/thollander/actions-comment-pull-request but not sure if this can update comments too rather than creating new ones

Copy link
Contributor

Choose a reason for hiding this comment

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

it appears it can update comments too? I'd use it if it can.

In the original CI i wrote this CI file for, i can't do that since the comment keeps some more state with it as well, but here i should be able to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice https://github.com/thollander/actions-comment-pull-request seems to be working well.
It apparently works the same way my curl stuff were working, but it clears 80 lines out of the CI file so better to use this instead.

Copy link

github-actions bot commented Jan 14, 2025

Benchmark Report

✅ Pull request has no significant performance differences ✅

Click to expand comparison result

Benchmark check running at 2025-01-14 16:00:22 UTC

The baseline benchmarks is EQUAL to the defined thresholds.

Click to expand benchmark result

Baseline benchmarks

Host 250a58874101 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) * 550 551 560 570 580 590 590 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 520 530 530 530 590 590 20

Serializer

100xSerializerCPUTime_1024Parts

Metric p0 p25 p50 p75 p90 p99 p100 Samples
Time (user CPU) (ms) * 210 210 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

@ptoffy ptoffy merged commit 45bae90 into main Jan 14, 2025
13 checks passed
@ptoffy ptoffy deleted the benchmarks branch January 14, 2025 16:06
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