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

Disable the async pipeline by default #500

Merged
merged 4 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 25 additions & 16 deletions examples/with_winit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,16 @@ struct Args {
/// Use `0` for an automatic choice
#[arg(long, default_value_t=default_threads())]
num_init_threads: usize,
/// Use the asynchronous pipeline (if available) for rendering
///
/// The asynchronous pipeline is one approach for robust memory - see
/// <https://github.com/linebender/vello/issues/366>
///
/// However, it also has potential latency issues, especially for
/// accessibility technology, as it (currently) blocks the main thread for
/// extended periods
#[arg(long)]
async_pipeline: bool,
}

fn default_threads() -> usize {
Expand Down Expand Up @@ -451,8 +461,9 @@ fn run(
.surface
.get_current_texture()
.expect("failed to get surface texture");
#[cfg(not(target_arch = "wasm32"))]
{
// Note: we don't run the async/"robust" pipeline, as
// it requires more async wiring for the readback.
Copy link
Member

Choose a reason for hiding this comment

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

Could we get more detail here? What is meant by "async wiring" - Is this a limitation of !Send futures, single-threaded wasm, etc. - What exactly is the issue on WASM that we aren't doing async pipelines there?

Copy link
Member Author

Choose a reason for hiding this comment

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

The thread is #gpu > async on wasm. Broadly, the issue is: if we spawn a future, that needs to keep hold of the scene. But that's incompatible with lifetimes of keeping the scene around (to reuse allocations, etc.)

In theory, we should do this much more carefully, but we haven't yet. That might also be useful in contexts outside of browsers too, allowing multi-threaded scene rendering

if args.async_pipeline && cfg!(not(target_arch = "wasm32")) {
scene_complexity = vello::block_on_wgpu(
&device_handle.device,
renderers[render_state.surface.dev_id]
Expand All @@ -467,21 +478,19 @@ fn run(
),
)
.expect("failed to render to surface");
} else {
renderers[render_state.surface.dev_id]
.as_mut()
.unwrap()
.render_to_surface(
&device_handle.device,
&device_handle.queue,
&scene,
&surface_texture,
&render_params,
)
.expect("failed to render to surface");
}
// Note: in the wasm case, we're currently not running the robust
// pipeline, as it requires more async wiring for the readback.
#[cfg(target_arch = "wasm32")]
renderers[render_state.surface.dev_id]
.as_mut()
.unwrap()
.render_to_surface(
&device_handle.device,
&device_handle.queue,
&scene,
&surface_texture,
&render_params,
)
.expect("failed to render to surface");
surface_texture.present();
device_handle.device.poll(wgpu::Maintain::Poll);

Expand Down
17 changes: 17 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,12 +362,26 @@ impl Renderer {
occlusion_query_set: None,
timestamp_writes: None,
});
let mut render_pass = self
.profiler
.scope("blit to surface", &mut render_pass, device);
render_pass.set_pipeline(&blit.pipeline);
render_pass.set_bind_group(0, &bind_group, &[]);
render_pass.draw(0..6, 0..1);
}
#[cfg(feature = "wgpu-profiler")]
self.profiler.resolve_queries(&mut encoder);
queue.submit(Some(encoder.finish()));
self.target = Some(target);
#[cfg(feature = "wgpu-profiler")]
self.profiler.end_frame().unwrap();
#[cfg(feature = "wgpu-profiler")]
if let Some(result) = self
.profiler
.process_finished_frame(queue.get_timestamp_period())
{
self.profile_result = Some(result);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg(feature = "wgpu-profiler")]
self.profiler.end_frame().unwrap();
#[cfg(feature = "wgpu-profiler")]
if let Some(result) = self
.profiler
.process_finished_frame(queue.get_timestamp_period())
{
self.profile_result = Some(result);
}
#[cfg(feature = "wgpu-profiler")]
{
self.profiler.end_frame().unwrap();
if let Some(result) = self
.profiler
.process_finished_frame(queue.get_timestamp_period())
{
self.profile_result = Some(result);
}
}

Ok(())
}

Expand Down Expand Up @@ -510,6 +524,9 @@ impl Renderer {
timestamp_writes: None,
occlusion_query_set: None,
});
let mut render_pass = self
.profiler
.scope("blit to surface", &mut render_pass, device);
render_pass.set_pipeline(&blit.pipeline);
render_pass.set_bind_group(0, &bind_group, &[]);
render_pass.draw(0..6, 0..1);
Expand Down
5 changes: 4 additions & 1 deletion src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,13 @@ impl std::task::Wake for NullWake {
///
/// This will deadlock if the future is awaiting anything other than GPU progress.
pub fn block_on_wgpu<F: Future>(device: &Device, mut fut: F) -> F::Output {
if cfg!(target_arch = "wasm32") {
panic!("Blocking can't work on WASM, so");
}
let waker = std::task::Waker::from(std::sync::Arc::new(NullWake));
let mut context = std::task::Context::from_waker(&waker);
// Same logic as `pin_mut!` macro from `pin_utils`.
let mut fut = unsafe { std::pin::Pin::new_unchecked(&mut fut) };
let mut fut = std::pin::pin!(fut);
loop {
match fut.as_mut().poll(&mut context) {
std::task::Poll::Pending => {
Expand Down