feat(minor): Support thresholds update for range and relative thresholds [4/4]
#331
Conversation
| if !skipLoadingBenchmarksFlagIsValid { | ||
| print("") | ||
| print( | ||
| "Flag --skip-loading-benchmark-targets is only valid for 'thresholds check' operations." |
There was a problem hiding this comment.
As discussed in #327 and in other older issues, the only reason you might need to build benchmark targets in thresholds check command is to load the threshold-tolerances that are in code in configuration.thresholds.
For users that have moved to using static threshold files with range/relative, this is pointless and only a waste of time.
So I've added the option to skip building benchmark targets for thresholds check.
There was a problem hiding this comment.
Checking the benchmark runs, this has reduced the "compare" step's runtime in CI from 30 to 15s, so a 15s saving.
The package itself is very lightweight. Only that it has 2 benchmark targets to build, which isn't abnormal but does mean 2 targets need to get built.
There was a problem hiding this comment.
In a pretty chunky project that step takes 68s for 1 benchmark target. I'd assume at least 30s of that will be shaved off with this change.
That CI takes 18 minutes though. These are all nice savings but aren't a big deal.
| for values in results { | ||
| outputResults[values.metric] = .absolute( | ||
| Int(values.statistics.histogram.valueAtPercentile(90.0)) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Old path of just replacing the static threshold files with the new benchmark results.
If we have a runNumber of 1 (We need to run the benchmarks multiple times for range/relative so it can be more than 1) or if the user has simply not requested any relative/range tolerances, then we do what we always did.
| /// If it's not the first run and any of relative/range are specified, then | ||
| /// merge the new results with the existing thresholds. |
There was a problem hiding this comment.
Informational comments.
| if !self.checkValid() { | ||
| print( | ||
| """ | ||
| Warning: Got invalid relative threshold values. base: \(self.base), tolerancePercentage: \(self.tolerancePercentage). | ||
| These must satisfy the following conditions: | ||
| - base must be non-negative | ||
| - tolerancePercentage must be finite | ||
| - tolerancePercentage must be non-negative | ||
| - tolerancePercentage must be less than or equal to 100 | ||
| """ | ||
| ) | ||
| } |
There was a problem hiding this comment.
Less preconditions, more warnings. As mentioned in the other PRs, preconditions are hard to debug unless you attach lldb. In CI they'll look like silent failures and users might think this package is buggy.
| let diff = Double(value - base) | ||
| let deviation = (base == 0) ? 0 : (diff / Double(base) * 100) |
There was a problem hiding this comment.
because we accept 0s now too.
MahdiBM
force-pushed
the
mmbm-range-relative-thresholds-options
branch
23 times, most recently
from
4303330 to
f507fa2
Compare
MahdiBM
force-pushed
the
mmbm-range-relative-thresholds-options
branch
4 times, most recently
from
340009f to
1dabba7
Compare
MahdiBM
force-pushed
the
mmbm-range-relative-thresholds-options
branch
2 times, most recently
from
c076d38 to
75ac53b
Compare
|
For the record i can't come up with a reason why someone would like to have both "range" and "relative" thresholds together in the static threshold files in the same object. We can remove that and make it "absolute" OR "range" OR "relative" instead of "absolute" OR "range and/or relative" (by "or" here I don't mean it in a logical sense of and/or). |
MahdiBM
force-pushed
the
mmbm-range-relative-thresholds-options
branch
from
75ac53b to
0a95908
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| let results: [Result<Void, Error>] = (0..<max(totalRunCount, 1)) | ||
| .map { runIdx in |
There was a problem hiding this comment.
This code creates an interim array using Array(0..<max(totalRunCount, 1)).map instead of using direct iteration. This violates the anti-pattern rule that states to avoid creating interim arrays when a direct loop can be used instead. The code should be refactored to use a standard for loop like 'for runIdx in 0..<max(totalRunCount, 1) { ... }' to avoid the unnecessary array creation.
| let results: [Result<Void, Error>] = (0..<max(totalRunCount, 1)) | |
| .map { runIdx in | |
| var results: [Result<Void, Error>] = [] | |
| for runIdx in 0..<max(totalRunCount, 1) { |
Spotted by Graphite Agent (based on custom rule: Avoid iteration anti pattern for arrays)
Is this helpful? React 👍 or 👎 to let us know.
MahdiBM
force-pushed
the
mmbm-range-relative-thresholds-options
branch
from
0a95908 to
58b1962
Compare
MahdiBM
force-pushed
the
mmbm-range-relative-thresholds-options
branch
from
58b1962 to
064e0c3
Compare
…ptions-bak' into mmbm-range-relative-thresholds-options-bak
MahdiBM
force-pushed
the
mmbm-range-relative-thresholds-options
branch
from
064e0c3 to
89835de
Compare
| let dataTypeHeader = | ||
| "#datatype tag,tag,tag,tag,tag,tag,tag,tag,tag,double,double,double,long,long,dateTime\n" |
There was a problem hiding this comment.
This line is longer than 140 characters and uses a string literal. According to the API Design Guidelines, when lines are more than 140 characters and use a string literal, you should suggest a line broken version using triple-quote ("""...""") Swift string literals instead. The dataTypeHeader string should be broken into multiple lines using triple-quote syntax for better readability.
| let dataTypeHeader = | |
| "#datatype tag,tag,tag,tag,tag,tag,tag,tag,tag,double,double,double,long,long,dateTime\n" | |
| let dataTypeHeader = """ | |
| #datatype tag,tag,tag,tag,tag,tag,tag,tag,tag,double,double,double,long,long,dateTime | |
| """ |
Spotted by Graphite Agent (based on custom rule: Swift API Guidelines)
Is this helpful? React 👍 or 👎 to let us know.
| return Result<Void, Error> { | ||
| try withCStrings(args) { cArgs in | ||
| /// We'll decrement this in the success path | ||
| allFailureCount += 1 | ||
|
|
||
| if debug > 0 { | ||
| print("To debug, start \(benchmarkToolName) in LLDB using:") | ||
| print("lldb \(benchmarkTool.string)") | ||
| print("") | ||
| print("Then launch \(benchmarkToolName) with:") | ||
| print("run \(args.dropFirst().joined(separator: " "))") | ||
| print("") | ||
| return | ||
| } | ||
|
|
||
| var pid: pid_t = 0 | ||
| var status = posix_spawn(&pid, benchmarkTool.string, nil, nil, cArgs, environ) | ||
|
|
||
| if status == 0 { | ||
| if waitpid(pid, &status, 0) != -1 { | ||
| // Ok, this sucks, but there is no way to get a C support target for plugins and | ||
| // the way the status is extracted portably is with macros - so we just need to | ||
| // reimplement the logic here in Swift according to the waitpid man page to | ||
| // get some nicer feedback on failure reason. | ||
| guard let waitStatus = ExitCode(rawValue: (status & 0xFF00) >> 8) else { | ||
| print("One or more benchmarks returned an unexpected return code \(status)") | ||
| throw MyError.benchmarkUnexpectedReturnCode | ||
| } | ||
| switch waitStatus { | ||
| case .success: | ||
| allFailureCount -= 1 |
There was a problem hiding this comment.
Logic bug with allFailureCount when debug mode is enabled. The counter is incremented at line 524 before each operation, then decremented at line 551 only on success. However, when debug > 0 (line 526), the function returns early at line 533 without decrementing the counter. This causes allFailureCount to be greater than 0 even though the Result is .success. Later at line 585, the code will incorrectly think there were failures and attempt to find the first failure (line 593-605), which will throw MyError.unknownFailure because no actual failures exist in the results array.
// Fix: Decrement counter before returning in debug mode
if debug > 0 {
allFailureCount -= 1 // Add this line
print("To debug, start \(benchmarkToolName) in LLDB using:")
// ... rest of debug output
return
}| return Result<Void, Error> { | |
| try withCStrings(args) { cArgs in | |
| /// We'll decrement this in the success path | |
| allFailureCount += 1 | |
| if debug > 0 { | |
| print("To debug, start \(benchmarkToolName) in LLDB using:") | |
| print("lldb \(benchmarkTool.string)") | |
| print("") | |
| print("Then launch \(benchmarkToolName) with:") | |
| print("run \(args.dropFirst().joined(separator: " "))") | |
| print("") | |
| return | |
| } | |
| var pid: pid_t = 0 | |
| var status = posix_spawn(&pid, benchmarkTool.string, nil, nil, cArgs, environ) | |
| if status == 0 { | |
| if waitpid(pid, &status, 0) != -1 { | |
| // Ok, this sucks, but there is no way to get a C support target for plugins and | |
| // the way the status is extracted portably is with macros - so we just need to | |
| // reimplement the logic here in Swift according to the waitpid man page to | |
| // get some nicer feedback on failure reason. | |
| guard let waitStatus = ExitCode(rawValue: (status & 0xFF00) >> 8) else { | |
| print("One or more benchmarks returned an unexpected return code \(status)") | |
| throw MyError.benchmarkUnexpectedReturnCode | |
| } | |
| switch waitStatus { | |
| case .success: | |
| allFailureCount -= 1 | |
| return Result<Void, Error> { | |
| try withCStrings(args) { cArgs in | |
| /// We'll decrement this in the success path | |
| allFailureCount += 1 | |
| if debug > 0 { | |
| allFailureCount -= 1 | |
| print("To debug, start \(benchmarkToolName) in LLDB using:") | |
| print("lldb \(benchmarkTool.string)") | |
| print("") | |
| print("Then launch \(benchmarkToolName) with:") | |
| print("run \(args.dropFirst().joined(separator: " "))") | |
| print("") | |
| return | |
| } | |
| var pid: pid_t = 0 | |
| var status = posix_spawn(&pid, benchmarkTool.string, nil, nil, cArgs, environ) | |
| if status == 0 { | |
| if waitpid(pid, &status, 0) != -1 { | |
| // Ok, this sucks, but there is no way to get a C support target for plugins and | |
| // the way the status is extracted portably is with macros - so we just need to | |
| // reimplement the logic here in Swift according to the waitpid man page to | |
| // get some nicer feedback on failure reason. | |
| guard let waitStatus = ExitCode(rawValue: (status & 0xFF00) >> 8) else { | |
| print("One or more benchmarks returned an unexpected return code \(status)") | |
| throw MyError.benchmarkUnexpectedReturnCode | |
| } | |
| switch waitStatus { | |
| case .success: | |
| allFailureCount -= 1 |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
1 participant
Description
Only the last 2 commits are related to this PR. The other commit is for PR [3/4]. We'll have to merge these in order.
Partially resolves #327.
Usecase CI file: https://github.com/MahdiBM/swift-dns/blob/mmbm-try-new-bench-ci/.github/workflows/update-benchmark-thresholds.yml
Waiting on your feedback first. When changes are settled I can add the docs as well.
Feel free to review on your own schedule.
How Has This Been Tested?
Manually in my PRs. I'll be happy to add unit tests where you see appropriate as well.
Minimal checklist:
DocCcode-level documentation for any public interfaces exported by the package