initial implementation

[jlizen]

Sharing initial commit for design feedback. Please pick it to pieces. Glad to split into smaller commits if it'd be helpful. I figured it was short enough that seeing the whole picture might be useful.

Some specific questions:

Crate name

Is compute-heavy-future-executor too long? Most crates are like 6 letters long. I was trying to make it clear what it does, but it's pretty klunky. Open to alternatives

Strategy-setting API

I don't love the current ergonomics of initializing a strategy. We have all these calls like initialize_block_in_place_strategy() which either initialize the oncelock or panic.

I was considering something like a builder or an enum, but it felt ridiculous given that there is only really one step for these and then we stash any output in the once lock. And then they are more verbose to call.

Other ideas welcome.

Custom executor

I wanted to put in an escape hatch to allow alternative async runtimes / customizing existing strategies with extra metrics or whatever / etc. But, the current form isn't particularly pleasant.

Ideally the caller would be able to implement the ComputeHeavyFutureExecutor trait themselves, but there were a few issues:

• We can't just accept a generic since we need to store the executor in the once lock, and we can't have generics in the static context
• We can't accept Box<dyn ComputeHeavyFutureExecutor> because the trait isn't object safe to its own generic bounds on its execute(fut: F) method

Instead I stored a closure inside of the CustomExecutor struct. That closure takes a future with Any output on the way in, and returns one with Any output. Because our execute call does have concrete typing, we can temporarily type erase the future's output before sending it into the closure, and then downcast it back on the flip side.

I think it's sound (please correct me, of course). Unfortunately we only are validating that the closure we get doesn't mangle the type, on initialization, not at compile time.

In addition to complexity, this also adds quite a few extra allocations due to all the added vtables and such.

I'm open to alternative approaches. Or, if it's better just to drop this functionality from the library entirely, and force people to stay on the rails, I'm open to that too.

Using get_or_init() inside spawn_compute_heavy_future()

Feedback on some dummy code in rustls/tokio-rustls#94 (comment) was that get_or_init() was overkill top call from inside the spawn handling.

As best as I can tell, using get_or_init() rather than matching against the runtime flavor every time this function is called, is strictly more efficient - since, it anyway starts off with OnceCell::get(), and then only calls the secondary logic if it was uninitialized - ref https://doc.rust-lang.org/beta/src/std/sync/once_lock.rs.html#387

This means that with get_or_init(), in case we need to use defaults, we have the single get branch every call, and the first call, the additional initialization logic.

Meanwhile with just a naked get().unwrap_or_else(), we still have the single get branch every call, and then the additional match branch against runtime flavor.

Probably a minor point either way, just calling out my reasoning so that it can be corrected as needed :)

Use of log crate

It felt strange depending on tracing when tokio itself is optional. I was browsing other libraries and was seeing that many of them just use the simple log crate. Which has interop with tracing anyway.

Is that the right choice? Or are libraries just using log because they were written before tracing was widely used?

Testing

Unit tests + doc tests succeed locally, with and without the tokio cfg flag enabled. I didn't want to overload this PR even further by setting some github actions, but I'll get to it!

[arielb1]

Is compute-heavy-future-executor too long? Most crates are like 6 letters long. I was trying to make it clear what it does, but it's pretty klunky. Open to alternatives

I don't think a long name now is a problem.

I don't love the current ergonomics of initializing a strategy. We have all these calls like initialize_block_in_place_strategy() which either initialize the oncelock or panic.

I believe you should return a Result rather than panicking. A framework-like library might want to supply a strategy without panicking.

A builder might be nicer to find references for. For example, one that is used like compute_heavy_future::global_strategy().initialize_block_in_place() form.

I think it's sound (please correct me, of course).

If you don't use unsafe then it has to be sound. It does add a few extra allocations tho [I think 3 - one for the future, one for boxing the output, one for the waiter], but these should not be bad for futures that are actually "compute-heavy".

Meanwhile with just a naked get().unwrap_or_else(), we still have the single get branch every call, and then the additional match branch against runtime flavor.

This should not affect performance significantly. In any interesting case both branches should be perfectly predictable.

[jlizen]

@arielb1 - all comments should be addressed, thanks again for the close read

The main thing I want to call out is, I ended up adding a secondary OnceLock for the default strategy, that is lazily set and then subsequently ignored if a strategy is initialized afterwards.

I needed this because I started having lifetime issues on the Default impl for ExecutorStrategyImpl once I added in the possible Semaphore usage, even though it is always None when constructed as a default.

I figured it was probably fine since it still avoids the sharp edge of libraries calling spawn_compute_heavy_future() before the caller initializes. Open to alternatives.

Otherwise, I tried to break changes into commits, though some got a bit bigger. Here's the rundown:

Changes since last review

• migrate from config flags to features
• Add global_strategy_builder() method
• Optional concurrency control for all executors
• All executors are cancellable
  • I considered adding a knob to remove this behavior, but figured it was scope creep and cancellable is the right default behavior. I can open a github issue to see if anybody is interested in that later.
• Moved the custom executor over to using a oneshot channel rather than having dyn Any
  • I considered keeping some support for Any in case the closure wants to know the future's output type for prioritization or other selective handling. But that's scope creep, I can cut a github issue to see if anybody cares about having support for that via a new executor / new knob.
• Removed the default to BlockInPlace strategy for multi-threaded executor, there are too many footguns with it. Anyway the caller can choose it explicitly.
• Moved tests to integration tests rather than requiring nextest, it's awkward because it forces a flat directory structure but it's still better than doctests I feel
• Uses COMPUTE_HEAVY_FUTURE_EXECUTOR_STRATEGY::get() rather than get_or_init() inside spawn_compute_heavy_future() (though there is now a secondary oncelock for the default strategy as mentioned above)
• Various other small cleanup / tweaks