Skip to content
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

Add BijectiveDictionaryBenchmark #22

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

DandyLyons
Copy link
Collaborator

@DandyLyons DandyLyons commented Sep 12, 2024

This PR adds benchmark testing using https://github.com/apple/swift-collections-benchmark/tree/main
Resolves issue #14

Context

Helpful resources:

  1. https://github.com/apple/swift-collections-benchmark/blob/main/Documentation/01%20Getting%20Started.md
  2. https://github.com/pointfreeco/swift-identified-collections/blob/main/Sources/swift-identified-collections-benchmark/main.swift

Usage

To run benchmarks you can use:

swift run -c release BijectiveDictionaryBenchmark run --cycles 3 results

To chart benchmarks you can use:

swift run -c release BijectiveDictionaryBenchmark render results chart.png   

For more info see: https://github.com/apple/swift-collections-benchmark/blob/main/Documentation/01%20Getting%20Started.md#running-benchmarks

Request For Comment

  1. I think it would be helpful to compare at least some metrics to Dictionary. This can help us internally, as well as inform users of performance tradeoffs.
  2. When ready, I think we should add a Performance section to the README, with a graph of the benchmarks. Similar to this: https://github.com/pointfreeco/swift-identified-collections/tree/main#performance

Room For Further Improvement

  1. I will add benchmark tests for lookups, removals, etc.
  2. In another PR, we should probably add CI.

Note

swiftLanguageVersions is deprecated in Package.swift after Swift 6. We should use swiftLanguageModes instead.

@DandyLyons DandyLyons requested a review from ladvoc September 12, 2024 18:20
@ladvoc ladvoc mentioned this pull request Sep 12, 2024
bDict[left: element.left] = element.right
}
return bDict
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functions are now written directly inline in the benchmark tests to improve readability.

@DandyLyons
Copy link
Collaborator Author

Discovery: In the standard library, Dictionary has a suite of benchmark tests located at single-source. We can learn from these. They're using a different benchmarking library.

One thing I noticed is that they use @inline(never). It's possible that we should use this. However, I think it's already covered by the blackhole() method.

(You don't want to inline your benchmark tests because you want to guarantee that the compiler isn't artificially skewing your results by optimizing it.)

@DandyLyons
Copy link
Collaborator Author

There's still a lot of work to be done on the benchmarks. They haven't been thoroughly checked for correctness yet. But I though you would be interested in seeing some of the initial results.

chart

@ladvoc
Copy link
Owner

ladvoc commented Sep 14, 2024

I absolutely agree. The goal of benchmarking should be to understand and quantify the overhead BijectiveDictionary adds over Dictionary—both for making optimizations internally and for informing users of this library. Ultimately, I think it is a good idea to benchmark and compare all common operations between the two types.

I think we should first implement and test all key features, reaching feature parity with Dictionary, and then utilize benchmarks to make optimizations, using Dictionary as a performance baseline.

@ladvoc
Copy link
Owner

ladvoc commented Sep 14, 2024

Discovery: In the standard library, Dictionary has a suite of benchmark tests located at single-source. We can learn from these. They're using a different benchmarking library.

One thing I noticed is that they use @inline(never). It's possible that we should use this. However, I think it's already covered by the blackhole() method.

(You don't want to inline your benchmark tests because you want to guarantee that the compiler isn't artificially skewing your results by optimizing it.)

Yes, it looks like this is covered by the blackhole() method and we safely omit @inline(never).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants