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

fix(patch): Use @_optimize(none) instead of @inline(never) for blackHole to ensure it's not broken by cross-module optimisation in Swift 5.8 #179

Closed hassila closed 1 year ago

hassila commented 1 year ago

Description

blackHole() didn't work as expected for some of our benchmarks, some further investigation found this commit: https://github.com/apple/swift/commit/1fceeab71e79dc96f1b6f560bf745b016d7fcdcf

AFAIU this broke with Swift 5.8.

Using the same approach for our benchmark and we got more expected results inline.

How Has This Been Tested?

Manual tested with internal benchmarks.

Minimal checklist:

github-actions[bot] commented 1 year ago

Pull request benchmark comparison [ubuntu-latest] with 'main' run at 2023-08-22T10:25:41+00:00 Pull request had performance regressions

codecov[bot] commented 1 year ago

Codecov Report

Merging #179 (b9c1d11) into main (8cc2be5) will increase coverage by 0.29%. The diff coverage is 100.00%.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179/graphs/tree.svg?width=650&height=150&src=pr&token=hXHmhEG1iF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one)](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one) ```diff @@ Coverage Diff @@ ## main #179 +/- ## ========================================== + Coverage 71.01% 71.29% +0.29% ========================================== Files 26 27 +1 Lines 3470 3501 +31 ========================================== + Hits 2464 2496 +32 + Misses 1006 1005 -1 ``` | [Files Changed](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one) | Coverage Δ | | |---|---|---| | [Sources/Benchmark/Benchmark.swift](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one#diff-U291cmNlcy9CZW5jaG1hcmsvQmVuY2htYXJrLnN3aWZ0) | `73.05% <ø> (+0.71%)` | :arrow_up: | | [Sources/Benchmark/Blackhole.swift](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one#diff-U291cmNlcy9CZW5jaG1hcmsvQmxhY2tob2xlLnN3aWZ0) | `25.00% <ø> (ø)` | | | [Tests/BenchmarkTests/AdditionalTests.swift](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one#diff-VGVzdHMvQmVuY2htYXJrVGVzdHMvQWRkaXRpb25hbFRlc3RzLnN3aWZ0) | `100.00% <100.00%> (ø)` | | | [Files Changed](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one) | Coverage Δ | | |---|---|---| | [Sources/Benchmark/Benchmark.swift](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one#diff-U291cmNlcy9CZW5jaG1hcmsvQmVuY2htYXJrLnN3aWZ0) | `73.05% <ø> (+0.71%)` | :arrow_up: | | [Sources/Benchmark/Blackhole.swift](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one#diff-U291cmNlcy9CZW5jaG1hcmsvQmxhY2tob2xlLnN3aWZ0) | `25.00% <ø> (ø)` | | | [Tests/BenchmarkTests/AdditionalTests.swift](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one#diff-VGVzdHMvQmVuY2htYXJrVGVzdHMvQWRkaXRpb25hbFRlc3RzLnN3aWZ0) | `100.00% <100.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one). Last update [8cc2be5...b9c1d11](https://app.codecov.io/gh/ordo-one/package-benchmark/pull/179?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ordo-one).
hassila commented 1 year ago

Should add a unit test to the benchmark package that validates that blackHole works as expected so we don't run into this again, will do that tomorrow.