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

Delayed queuing of spawn_local tasks #2854

Merged
merged 6 commits into from
Apr 7, 2022

Conversation

WorldSEnder
Copy link
Contributor

Fixes #2847

Tasks are now reliably scheduled to start running on the next tick reliably. Note that Tasks that later wake on the same tick as they are, still run immediately without this delay.

crates/futures/src/task/singlethread.rs Outdated Show resolved Hide resolved
front_tasks: RefCell<VecDeque<Rc<crate::task::Task>>>,
// We use a double-buffer approach to running tasks. Tasks initially enter the
// back_tasks which gets swapped to the front when we resume spinning.
back_tasks: RefCell<VecDeque<Rc<crate::task::Task>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not actually sure if it's necessary to have a distinction here with two deques. Could the new tasks actually always be enqueued for a future microtask tick? I think implementing everything in the same microtask tick was primarily done for convenience over anything else.

Copy link
Contributor Author

@WorldSEnder WorldSEnder Apr 5, 2022

Choose a reason for hiding this comment

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

I wasn't sure how common it was for tasks to immediately reschedule. I thought it could lead to a surprising number of ticks for some tasks to run to completion if rescheduling always waits for the next tick (compared to the current behaviour).

Note the test case actually tests for this, by requiring the Yield task to complete (on the same tick) before the newly scheduled task (on the next tick).

Re-reading the documentation of js Promise, which always schedules the callbacks of .then() asynchronously, delaying would be the choice, but besides the existing quirks of javascript, I can't think of a reason to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's safe to always enqueue to a future tick because that's what JS promises do with .then and once you're using promises chances are that things will take awhile to complete and it'll be pretty rare to have an immediately enqueued piece of work anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has been a bit more complicated to write a good test case, but I think I got it. The test case fails on main and succeeds with both approaches

@WorldSEnder
Copy link
Contributor Author

The one failing check seems to be unrelated to the PR and more to the release of wasmprinter.

@alexcrichton
Copy link
Contributor

Could you either pin the wasmprinter version to the previous version or run the failing test suite with BLESS=1 (I forget the exact env var name) to get CI passing?

@alexcrichton alexcrichton merged commit 65105ff into rustwasm:main Apr 7, 2022
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.

Using spawn_local inside a task itself spawn with spawn_local will not run on the next microtick
2 participants