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

Push ComponentState into the scheduler instead of boxed runners #2485

Open
1 of 3 tasks
WorldSEnder opened this issue Mar 2, 2022 · 2 comments
Open
1 of 3 tasks

Push ComponentState into the scheduler instead of boxed runners #2485

WorldSEnder opened this issue Mar 2, 2022 · 2 comments
Labels
A-yew Area: The main yew crate ergonomics performance

Comments

@WorldSEnder
Copy link
Member

Suggestion

The scheduler for component lifecycles currently pushes boxed runners into the queue, incurring an allocation. A stable memory allocation is already available, in the form of a Rc<ComponentState> for each component. This could be used to store the pending lifecycle events similar to the MsgQueue that was introduced as part of #2421. An implementation should not re-introduce the generics that were removed as part of that PR.

See also: #2330 (comment)

Questionnaire

  • I'm interested in fixing this myself but don't know where to start
  • I would like to fix and I have a solution
  • I don't have time to fix this right now, but maybe later
@WorldSEnder WorldSEnder added the bug label Mar 2, 2022
@WorldSEnder WorldSEnder added ergonomics performance A-yew Area: The main yew crate and removed bug labels Mar 11, 2022
@przembot
Copy link

Hi, interested in picking this up, which solution should be preferred?

  1. Replace Box<dyn Runnable> in scheduler with enum Task which would contain task kind, Rc<ComponentState> and relevant data and matched runners for each Task variant.
  2. Replace scheduler queues with a main one with Rc<ComponentState>, where the ComponentState would contain a queue of tasks to be done on the component. This would though remove the scheduling depending on the task kind which is now present

@WorldSEnder
Copy link
Member Author

WorldSEnder commented Mar 21, 2022

Hi, interested in picking this up, which solution should be preferred?

1. Replace `Box<dyn Runnable>` in scheduler with enum `Task` which would contain task kind, `Rc<ComponentState>` and relevant data and matched runners for each `Task` variant.

2. Replace scheduler queues with a main one with `Rc<ComponentState>`, where the ComponentState would contain a queue of tasks to be done on the component. This would though remove the scheduling depending on the task kind which is now present

The different task queues should be retained and run in the same order. Note that each task queue, except for create (generic over COMP) and main (for user tasks) contains only a single type of task, hence Boxing those is unnecessary.

Since I opened the issue, the implementation has changed a bit. Most impls then were generic in the type of BaseComponent like CreateRunner still is. Now, runners such as UpdateRunner are monomorphized, so the easiest solution is to e.g. replace destroy: Vec<Box<dyn Runnable>> with destroy: Vec<DestroyRunner>. Bonus points if you find a way to get rid of the generic argument in CreateRunner without adding more allocations, I think this should be possible, but might be a bit more fiddly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate ergonomics performance
Projects
None yet
Development

No branches or pull requests

2 participants