-
Notifications
You must be signed in to change notification settings - Fork 74
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
Refactor the execution to avoid setTimeout #2999
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.
twiggy diff reportDifference in .wasm size before and after this pull request.
|
Testing this PR is a bit complicated. EDIT: actually, still getting timeouts afterwards, so inconclusive. EDIT 2: it definitely works better after the PR |
Close #2996
This PR does many things:
Remove the
crate::util::yield_twice
function in favor of a platform-specific functionTPlat::yield_after_cpu_intensive
. This way, the implementation of the yielding, which is a bit hacky due to the requirements of the Wasm node, is specific to the Wasm node.Refactor the
nextJsonRpcResponse
function and thejsonRpcResponsesNonEmptyCallback
callback. Right now, when we wait for a JSON-RPC response to come, we wait for smoldot to notify that it has a response available. But because this notification is done through a callback, we can't immediately query the content of the response, so instead we usesetTimeout(..., 0)
to register a task that queries the content of the response and then notify thePromise
that was waiting. Now we instead immediately notify aPromise
, and thisPromise
then queries the content of the response as an aftermath.In the JS code, add a
registerShouldPeriodicallyYield
function whose implementation is "platform-specific" (browsers, NodeJS, Deno). This function controls a new setting called "should periodically yield". In NodeJS and Deno, this is alwaysfalse
. In the browser, this isfalse
if and only if the page is in the background andtrue
if it is in the foreground. Passingfalse
reduces the usage ofsetTimeout
, while passingtrue
promotes the usage ofsetTimeout
to avoid blocking the browser.In the JS <-> Rust bindings, add a
periodically_yield
parameter toinit
and add a newset_periodically_yield
function. See above point.Refactor the way the execution is done. Currently we spawn at initialization a task (using
setTimeout(..., 0)
) that runs everything. When something needs to be executed, this task wakes up by callingsetTimeout(..., 0)
again. After this PR, there is no "background task" anymore. Instead, theFuture
that drives everything is now put inCLIENT
variable, and every single binding function calls a newadvance_execution
function before returning.advance_execution
polls thatFuture
and, depending on theperiodically_yield
setting, either executes everything until there is nothing more to do, or executes until the first yield.All these changes eliminate the usage of
setTimeout
to the strict minimum, which is when we actually need a timer.