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

Dn loop bound box benchmark #2778

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

dnadoba
Copy link
Member

@dnadoba dnadoba commented Jul 10, 2024

Motivation:

@Lukasa and @weissi asked for a benchmark

Modifications:

add benchmark

Result:

----------------------------------------------------------------------------------------------------------------------------
NIOLoopBoundBox.value metrics
----------------------------------------------------------------------------------------------------------------------------

╒══════════════════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕
│             Malloc (total) *             │      p0 │     p25 │     p50 │     p75 │     p90 │     p99 │    p100 │ Samples │
╞══════════════════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡
│                 v2.68.0                  │    1001 │    1001 │    1001 │    1001 │    1001 │    1001 │    1001 │    6725 │
├──────────────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│               Current_run                │       1 │       1 │       1 │       1 │       1 │       1 │       1 │   10000 │
├──────────────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│                    Δ                     │   -1000 │   -1000 │   -1000 │   -1000 │   -1000 │   -1000 │   -1000 │    3275 │
├──────────────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│              Improvement %               │     100 │     100 │     100 │     100 │     100 │     100 │     100 │    3275 │
╘══════════════════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛

@dnadoba dnadoba enabled auto-merge (squash) July 10, 2024 16:59
boundBox.value.reserveCapacity(1000)

for i in 0..<1000 {
boundBox.value.append(i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we run this multiple times, for the scaling factor? Ideally we’d only count allocations in the loop body, as we’re otherwise very sensitive to the runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. This breaks the benchmarking comparison tool as it now thinks we have improved by 0% instead of 100% 😆

$ swift package benchmark baseline compare v2.68.0 --filter="NIOLoopBoundBox.value"  
[...]
----------------------------------------------------------------------------------------------------------------------------
NIOLoopBoundBox.value metrics
----------------------------------------------------------------------------------------------------------------------------

╒══════════════════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕
│             Malloc (total) *             │      p0 │     p25 │     p50 │     p75 │     p90 │     p99 │    p100 │ Samples │
╞══════════════════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡
│                 v2.68.0                  │    1001 │    1001 │    1001 │    1001 │    1001 │    1001 │    1001 │       8 │
├──────────────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│               Current_run                │       0 │       0 │       0 │       0 │       0 │       0 │       0 │     469 │
├──────────────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│                    Δ                     │   -1001 │   -1001 │   -1001 │   -1001 │   -1001 │   -1001 │   -1001 │     461 │
├──────────────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│              Improvement %               │       0 │       0 │       0 │       0 │       0 │       0 │       0 │     461 │
╘══════════════════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛

@hassila is this a known issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it seems it is normalizing to the slower benchmark instead of the reverse - looks like a plain bug but don't think we've got a case for it - out of office until August so will be a while before I can have a proper look. If you can, file a case would be great - otherwise I'll pick it up when back.

I would suggest using a scaling factor for your bench mark though and pass it on instead to the function - that way you can get normalized numbers per subiteration

dnadoba and others added 5 commits July 11, 2024 11:26
# Conflicts:
#	Benchmarks/Thresholds/nightly-next/NIOCoreBenchmarks.NIOLoopBoundBox.value.p90.json
# Motivation

We added new CI to run both nightly main and nightly next but forgot to update the benchmark thresholds script

# Modification

This PR updates the benchmark thresholds script and adds a nightly next docker compose image.

# Result

We can use the update benchmark thresholds script again.
…benchmark

# Conflicts:
#	dev/update-benchmark-thresholds.sh

for _ in benchmark.scaledIterations {
boundBox.value.removeAll(keepingCapacity: true)
for i in 0..<1000 {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only have the scaled iterations loop, not this inner one.

Copy link
Member Author

Choose a reason for hiding this comment

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

How does that protect against runtime allocations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, this is now no longer testing what I wanted to test: the performance of repeated mutation of value. CoW types like Array generally can have one off allocations but that isn't the real problem. The problem is if it happens on every iteration in a loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the future, the reason looping scaledIterations times helps is that the malloc values output are divided by scaledIterations. This means any allocation that doesn't happen every loop iteration is folded away, so spurious runtime allocations that may slightly differ between runs don't affect our thresholds.

dnadoba added 3 commits July 15, 2024 11:24
# Conflicts:
#	Benchmarks/Thresholds/5.10/NIOCoreBenchmarks.NIOAsyncChannel.init.p90.json
#	Benchmarks/Thresholds/5.8/NIOCoreBenchmarks.NIOAsyncChannel.init.p90.json
#	Benchmarks/Thresholds/5.9/NIOCoreBenchmarks.NIOAsyncChannel.init.p90.json
#	Benchmarks/Thresholds/nightly-main/NIOCoreBenchmarks.NIOAsyncChannel.init.p90.json
#	Benchmarks/Thresholds/nightly-next/NIOCoreBenchmarks.NIOAsyncChannel.init.p90.json
@dnadoba dnadoba disabled auto-merge July 15, 2024 15:50
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

I'm generally happy with this benchmark.

@FranzBusch FranzBusch added the semver/none No version bump required. label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants