-
Notifications
You must be signed in to change notification settings - Fork 651
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
base: main
Are you sure you want to change the base?
Conversation
…t-nio into dn-loop-bound-box-benchmark
boundBox.value.reserveCapacity(1000) | ||
|
||
for i in 0..<1000 { | ||
boundBox.value.append(i) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
# 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
# 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
There was a problem hiding this 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.
Motivation:
@Lukasa and @weissi asked for a benchmark
Modifications:
add benchmark
Result: