-
Notifications
You must be signed in to change notification settings - Fork 51
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
"Flatten" the tasks in the light client #349
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
twiggy diff reportDifference in .wasm size before and after this pull request.
|
tomaka
added a commit
to tomaka/smoldot
that referenced
this pull request
Mar 28, 2023
tomaka
added a commit
that referenced
this pull request
Mar 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc #345
The light client spawns various tasks that are run in the background.
It also creates two
FuturesUnordered
(one in the networking service and one in the JSON-RPC service), pushes tasks to it, and then spawns a background task that runs thisFuturesUnordered
.In other words, there's a "hierarchy" of tasks: the tasks spawned in the background, and the tasks that are run by tasks spawned in the background.
The advantage of doing so and the reason why this was done so is that the "second level" of the "hierarchy" doesn't have the same priority as the "first layer". The first layer contains important tasks, and the second layer the less important ones.
Unfortunately, the
FuturesUnordered
doesn't work well when it comes to yielding. It's only after tasks have yielded twice that theFuturesUnordered
yields. Because the wasm node implements background tasks with aFuturesUnordered
as well, this means that all tasks have to yield a total of 4 times in order to "actually" yield to the browser/NodeJS/Deno.At the moment, yielding is implemented by sleeping twice, for exactly this reason.
See rust-lang/futures-rs#2053 for a discussion about this.
In order to solve that problem, this PR removes the layers of hierarchy. This means that all tasks now have the same execution priority, which isn't great in the absolute but in practice isn't a problem. If we need to restore priority levels, I think a better solution is to add a parameter when spawning a background task indicating the priority.