search

ordo-one / package-benchmark

Swift benchmark runner with many performance metrics and great CI support
Apache License 2.0
326 stars 25 forks source link

Absolute Thresholds from Benchmark-Setup are not respected #282

Closed fred-sch closed 1 month ago

fred-sch commented 1 month ago

Running the following benchmark with swift package benchmark baseline check --check-absolute

    Benchmark(
        "p90Sleep" ,
        configuration: .init(metrics: .all, thresholds: [.wallClock: .init(absolute: [.p90: .milliseconds(100)])])
    ) { _ in
        sleep(1)
    }

I was expecting it to fail. Instead the output was just:

Baseline 'Current_run' is EQUAL to the defined absolute baseline thresholds. (--check-absolute)

I looked into it and it seems that because of BenchmarkTool+Operations#L170 only p90-thresholds from a file checkAbsolutePath are considered.

hassila commented 1 month ago

Thanks for the report, will soon be a significant update to threshold handling, will review this in that context - hope to have a PR up in the near future.

hassila commented 1 month ago

Should be explained by documentation and updated static threshold support in https://github.com/ordo-one/package-benchmark/pull/284

Basically what you define in the benchmark itself, is the threshold tolerance for checking, while an actual static threshold value (now) will be stored with swift package benchmark thresholds update ...

Please have a look and hope this clears things up.

fred-sch commented 1 month ago

Thanks for the quick reply!

I agree that https://github.com/ordo-one/package-benchmark/pull/284 should resolve/clarify the problem. I think mostly I was confused by the docs, the naming of the parameter (which you changed to thresholdTolerance now) and a missing error message when no p90 thresholds were found. I see all these things addressed in your PR!

hassila commented 1 month ago

Super, closing this then, thanks.