Remove unreliable SchedulingBenchmark
#2650
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: At the moment `SchedulingBenchmark` preheats single EL to a specified number of tasks. However, the actual performance test runs number of times, and EL doesn't get drained in between the runs. There are two problems with it: * perf test will trigger task heap dublings, which the preheating aims to avoid * each test run will be working with proportionally deeper heap, which means the run times are going to grow with each run Modifications: I plumbed through the # of runs to the `Benchmark.setUp`, and prepare ELG with # of ELs that match the expected number of runs. Result: Performance test will be more reliable.
weissi
reviewed
Feb 14, 2024
| for _ in 0..<self.numTasks { | ||
| self.loop.scheduleTask(in: .nanoseconds(0)) { | ||
| counter &+= 1 | ||
| func setUp(runs: Int) throws { |
There was a problem hiding this comment.
@gmilos instead of introducing the runs everywhere, can we not just use an ELG with just 1 thread? Then it should be fine, right?
There was a problem hiding this comment.
wait, hang on, it already tied it to exactly one EventLoop self.loop
There was a problem hiding this comment.
This whole test is completely busted. We need to either fix it (wait for the scheduled tasks) or just delete this perf test, currently it doesn't add value.
There was a problem hiding this comment.
yeah, here we can see that this is not useful
| schedule_100k_tasks | 0.063766844 | 0.105724529 | 0.07314636890000001 | 0.012951859063592227 |
|---|
min runtime: 0.063s
mean runtime: 0.073s
max runtime: 0.105s
std deviation: 0.012 s
that's way too high of a std dev to be useful
There was a problem hiding this comment.
@weissi where did you get these test results? #2650 (comment)
It looks like it's still the broken (3rd Jan re-copied) test results, right?
There was a problem hiding this comment.
I'd strongly suggest to delete this test. If we don't delete this test, then the only thing we can/should do is to spawn 1 EventLoopThread in each test's setUp. Relying on round robin and messing with existing groups (by enqueuing 100k tasks that will never run) is not a good idea, especially if there's a possibility that other perf tests are running on the same loop.
But again: I'd say we should delete the test, I don't think it adds value.
There was a problem hiding this comment.
@FranzBusch do you have opinions (seems like you added it originally in #2009)? If you're happy to delete, we can do that.
@gmilos that's the actual issue here. It should be completely drained after each run.
That doesn't sound ideal as if it's actually the case that the others aren't drained, then this will now cause high CPU load and expects us to have #runs CPUs available etc. |
|
@swift-nio-bot test perf please |
performance reportbuild id: 156 timestamp: Wed Jan 3 13:45:50 UTC 2024 results
comparison
significant differences found |
|
@swift-nio-bot perf test please |
No, because the tasks never run. They are just scheduled for some future date (that never arrives during the test run). So the tasks scheduled in the past runs are effectively dormant. |
gmilos
changed the title
SchedulingBenchmark preheating logic.SchedulingBenchmark
Lukasa
approved these changes
Mar 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
At the moment
SchedulingBenchmarkpreheats single EL to a specified number of tasks. However, the actual performance test runs number of times, and EL doesn't get drained in between the runs.There are two problems with it:
I proposed a fix, which made the benchmark work reliably, but @weissi and then others come to a consensus we're better off without it.
Modifications:
SchedulingBenchmarkremoved along with the dispatch site.Result: