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

Dynamically Allocated Executor #3558

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

PegasisForever
Copy link
Contributor

Implementation for #3334

@Dirbaio
Copy link
Member

Dirbaio commented Dec 2, 2024

I'm not sure that we should do this. My reasoning:

  • You can already use alloc to allocate tasks with the current public API with Box::leak(Box::new(TaskStorage::new())). This allows using generics and type inference and everything. Note this allows some things that the API in this PR doesn't allow:
    • You can reuse the storage by spawning it again when a task exits, if you keep the TaskStorage around.
    • You can use another allocator, it doesn't have to be Box. Anything that'll give you a &'static TaskStorage will work.
  • Dynamically allocated tasks still have severe downsides, mainly that you can't free the memory. This applies to both this PR and the TaskStorage API. This is unfixable, it's due to the core executor design (tasks living forever removes the need for refcounting wakers, which is what allows embassy-executor to be so fast+small). "Fixing" it would require adding refcounts and vtables so the executor design would basically collapse to the same as async-task.

I think no matter what we do embassy-executor is going to be a bad alloc executor, therefore we shouldn't encourage it. It's fine if it's possible to use TaskStorage with alloc, because it's "advanced" use that you have to go out of your way to do. Adding a more visible API like in this PR is basically telling people "hey go use this". They'll run into the limitation, or maybe not realize task storages are leaked and run into OOMs.

I do agree alloc has its use cases. We might want an embassy-alloc-executor crate, as a completely separate executor implementation. This way people would decide upfront whether they want alloc, picking between a GOOD alloc executor and a GOOD no-alloc executor. No weird limitations. The "alloc executor" would have the same design as async-task, fully refcounting tasks, or perhaps use async-task itself.

I do think we should expand docs on how to use TaskStorage with alloc. Crate docs, examples, FAQ entries would help. It's quite neat and there's little awareness of it.

@PegasisForever
Copy link
Contributor Author

@Dirbaio interesting, I'll experiment with the approaches you mentioned!

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