search

apple / swift-collections-benchmark

A benchmarking tool for Swift Collection algorithms
Apache License 2.0
340 stars 23 forks source link

Support Swift concurrency #19

Open robertmryan opened 10 months ago

robertmryan commented 10 months ago

A common benchmarking need is to benchmark asynchronous work. With the advent of Swift concurrency, I might suggest adding support for async closures.

With the current implementation, there are challenges:

  1. One has to use dispatch group/semaphore wait to make asynchronous work behave in the synchronous environment of this package. (See my note below for example of my clumsy workaround.)
  2. This package’s existing Task type is exceeding confusing for users who might attempt to use the now-pervasive Swift concurrency Task, getting non-standard autocompletion. One has to resort to import Concurrency and _Concurrency.Task.detached {…} (or whatever) when dealing with Swift concurrency Task.

I am happy to take a pass at a PR for this, but a few questions:

  1. How to handle this package’s existing Task type:

    • I might be inclined to rename this as BenchmarkTask or Benchmark.Test to avoid confusion. But that would be a breaking change. Is there a reason this is public and/or named Task? Thoughts regarding the renaming/namespacing?
    • Is there a reason that this Task was exposed rather than being internal/private?

  2. Preferences regarding the interface for renditions that take async closures?

    • Would you like an entirely separate async API? E.g., addSimpleAsync (or withSimple, or whatever) for async rendition of addSimple? Ditto with blackHole, etc. What naming convention would you like to adopt? How would you like to tackle this?

Conceptually, this does not seem outlandishly complicated to make this support concurrency, and the biggest question in my mind is how to do so elegantly in a non-breaking manner. If you would like me to attempt a PR, a little guidance would be appreciated. Of course, if you would rather tackle this yourself, that would be great. I’m happy either way, but was hesitant to spend a lot of time on this without a little direction and/or guidelines.

FWIW, here is an example of my benchmarking of asynchronous work. I had to wrap my asynchronous test in something that used a dispatch group (or I could have used a semaphore, too) to wait for the asynchronous work to finish. Using group/semaphore wait is clumsy and an anti-pattern in Swift concurrency. And the use of _Concurrency.Task.detached feels awkward.

lorentey commented 7 months ago

Interesting! I would love to figure out how to run repeatable benchmarks for async algorithms.

How to handle this package’s existing Task type:

This package has origins predating Swift Concurrency's Task type, which is why it is using that name.

This is not a source stable code base, and as you noted Task is rarely mentioned by clients, so we can certainly rename it to something else. Benchmark.Test is not bad -- let's go with that.

(If you do end up doing this, I think it would be good idea to split this off from any async additions, and land this renaming in a separate PR.)

Preferences regarding the interface for renditions that take async closures?

Yes, I'd indeed prefer to have asynchronous benchmarking use entirely separate measurement paths and separate APIs.

(I'd probably veto attempts to call any async functions or use any async infrastructure when running in "classic" mode, i.e. when measuring synchronous benchmarking tasks. (Unless I'd be convinced it wouldn't interfere with the reliability of measurements, of course!))

What if we had a separate AsyncBenchmark type to manage those?

Would you really need to define async versions of trivial helpers like blackHole or identity?