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

expose eager thread-local resource initialization on Engine #2946

Merged
merged 7 commits into from
Jun 4, 2021

Conversation

pchickey
Copy link
Contributor

Based on #2897 - draft until that lands

In the Tokio runtime and elsewhere, it is useful to eagerly initialize thread-local resources (signal handlers etc) to avoid taking a runtime hit after a service is started.

@pchickey pchickey requested a review from alexcrichton May 27, 2021 23:03
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator wasmtime:c-api Issues pertaining to the C API. labels May 27, 2021
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@peterhuene
Copy link
Member

peterhuene commented May 28, 2021

I think this might need to be rebased on main.

@peterhuene
Copy link
Member

Erm, never mind, I forgot we hadn't merged in the API changes yet, which this is based on.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

I'm a little wary to do this personally because it feels like we're exposing too many implementation details. For example I think it would be bad if something like a "wasmtime best practices" recommended this. I would personally prefer if we could make the per-thread initialization cheaper if the initialization takes too long, or solve it via some other means.

@pchickey pchickey force-pushed the pch/eager_per_thread_init branch from 200d303 to ad387f1 Compare June 3, 2021 22:09
@pchickey pchickey marked this pull request as ready for review June 3, 2021 22:15
@pchickey
Copy link
Contributor Author

pchickey commented Jun 3, 2021

That is understandable. We haven't measured, and I don't think signal handler installation takes especially long, but we wanted this functionality so that we could eliminate the possibility from any sort of performance issue we hit at runtime, or debugging any sort of interaction with other parts of the process that install signal handlers.

I don't feel extremely strongly about this but @acfoltzer advocated for it, and my judgement was the trouble it might save us debugging one day is worth exposing this implementation detail.

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jun 3, 2021
@github-actions
Copy link

github-actions bot commented Jun 3, 2021

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton
Copy link
Member

Ok, in any case it's a relatively small API and not super hard to support. Could you expand the documentation with some possible use cases though? I'd also prefer if the tokio example didn't get updated to use it because it feels like it's mostly just boilerplate for most use cases and not all that necessary. (although having this in a test seems good)

initialized = true;
}
p.set((state, initialized));
Ok(())
Copy link
Member

Choose a reason for hiding this comment

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

This could probably be a bit simpler with if initialized { return } and then p.set((state, true)) afterwards

@pchickey pchickey force-pushed the pch/eager_per_thread_init branch from ad387f1 to bf1d1a4 Compare June 4, 2021 18:22
@@ -63,6 +63,12 @@ impl Engine {
})
}

/// Eagerly initialize thread-local functionality shared by all [`Engine`]s. This
/// will be performed lazily by the runtime if users do not perform it eagerly.
Copy link
Member

Choose a reason for hiding this comment

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

As a suggestion for a doc block here:

Eagerly initialize thread-local functionality shared by all [Engine]s.

Wasmtime's implementation on some platforms may involve per-thread setup that needs to happen whenever WebAssembly is invoked. This setup can take on the order of a few hundred microseconds, whereas the overhead of calling WebAssembly is otherwise on the order of a few nanoseconds. This setup cost is paid once per-OS-thread. If your application is sensitive to the latencies of WebAssembly function calls, even those that happen first on a thread, then this function can be used to improve the consistency of each call into WebAssembly by explicitly frontloading the cost of the one-time setup per-thread.

Note that this function is not required to be called in any embedding. Wasmtime will automatically initialize thread-local-state as necessary on calls into WebAssembly. This is provided for use cases where the latency of WebAssembly calls are extra-important, which is not necessarily true of all embeddings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! That doc, and the bench docs, are now pushed.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

👍

I think though the last commit may have been missing the public-facing docs for the function by accident?

@pchickey
Copy link
Contributor Author

pchickey commented Jun 4, 2021

yep they were staged in git and i committed wrong.

@pchickey pchickey merged commit 38ab7a0 into main Jun 4, 2021
@pchickey pchickey deleted the pch/eager_per_thread_init branch June 4, 2021 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants