Skip to content
Open
Show file tree
Hide file tree
Changes from 9 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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions node-graph/interpreted-executor/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ pub fn wrap_network_in_scope(mut network: NodeNetwork, editor_api: Arc<PlatformE
DocumentNode {
call_argument: concrete!(Context),
inputs: vec![NodeInput::scope("editor-api"), NodeInput::node(NodeId(2), 0)],
implementation: DocumentNodeImplementation::ProtoNode(graphene_std::pixel_preview::pixel_preview::IDENTIFIER),
implementation: DocumentNodeImplementation::ProtoNode(graphene_std::render_pixel_preview::render_pixel_preview::IDENTIFIER),
context_features: graphene_std::ContextDependencies {
extract: ContextFeatures::FOOTPRINT | ContextFeatures::VARARGS,
inject: ContextFeatures::FOOTPRINT | ContextFeatures::VARARGS,
Expand All @@ -73,7 +73,7 @@ pub fn wrap_network_in_scope(mut network: NodeNetwork, editor_api: Arc<PlatformE
DocumentNode {
call_argument: concrete!(Context),
inputs: vec![NodeInput::scope("editor-api"), NodeInput::node(NodeId(3), 0)],
implementation: DocumentNodeImplementation::ProtoNode(graphene_std::render_node::render_background::IDENTIFIER),
implementation: DocumentNodeImplementation::ProtoNode(graphene_std::render_background::render_background::IDENTIFIER),
context_features: graphene_std::ContextDependencies {
extract: ContextFeatures::FOOTPRINT | ContextFeatures::VARARGS,
inject: ContextFeatures::empty(),
Expand Down
52 changes: 29 additions & 23 deletions node-graph/libraries/wgpu-executor/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,27 +1,28 @@
mod background; // TODO: Think about where to place this. Likely inlined in the node. Requires refactor of wgpu pipline usage.
mod context;
mod resample;
mod pipeline;
pub mod shader_runtime;
mod texture_cache;
pub mod texture_conversion;

use std::sync::Arc;
use std::any::{Any, TypeId};
use std::collections::HashMap;
use std::sync::{Arc, RwLock};

use crate::background::BackgroundCompositor;
use crate::resample::Resampler;
use crate::shader_runtime::ShaderRuntime;
use crate::texture_cache::TextureCache;
use anyhow::Result;
use core_types::Color;
use core_types::color::SRGBA8;
use futures::lock::Mutex;
use glam::{Affine2, UVec2};
use glam::UVec2;
use graphene_application_io::{ApplicationIo, EditorApi};
use vello::{AaConfig, AaSupport, RenderParams, Renderer, RendererOptions, Scene};
use wgpu::{Origin3d, TextureAspect};

pub use context::Context as WgpuContext;
pub use context::ContextBuilder as WgpuContextBuilder;
pub use pipeline::AsyncPipeline as AsyncWgpuPipeline;
pub use pipeline::Pipeline as WgpuPipeline;
pub use rendering::RenderContext;
pub use wgpu::Backends as WgpuBackends;
pub use wgpu::Features as WgpuFeatures;
Expand All @@ -33,8 +34,7 @@ pub struct WgpuExecutor {
pub context: WgpuContext,
texture_cache: Mutex<TextureCache>,
vello_renderer: Mutex<Renderer>,
resampler: Resampler,
background_compositor: BackgroundCompositor,
pipelines: RwLock<HashMap<TypeId, Arc<dyn Any + Send + Sync>>>,
pub shader_runtime: ShaderRuntime,
}

Expand Down Expand Up @@ -84,21 +84,30 @@ impl WgpuExecutor {
Ok(texture)
}

pub async fn resample_texture(&self, source: &wgpu::Texture, size: UVec2, transform: &glam::DAffine2) -> Arc<wgpu::Texture> {
let out = self.request_texture(size).await;
self.resampler.resample(&self.context, source, transform, &out);
out
pub async fn request_texture(&self, size: UVec2) -> Arc<wgpu::Texture> {
self.texture_cache.lock().await.request_texture(&self.context.device, size)
}

pub async fn composite_background(&self, foreground: &wgpu::Texture, backgrounds: &[rendering::Background], document_to_screen: Affine2, zoom: f32) -> Arc<wgpu::Texture> {
let size = foreground.size();
let output = self.request_texture(UVec2::new(size.width, size.height)).await;
self.background_compositor.composite(&self.context, foreground, &output, backgrounds, document_to_screen, zoom);
output
fn pipeline<P: WgpuPipeline>(&self) -> Arc<P> {
let key = TypeId::of::<P>();

let cached = self.pipelines.read().unwrap().get(&key).cloned();
if let Some(arc) = cached {
return arc.downcast::<P>().expect("TypeId<P> guarantees this downcast");
}

self.pipelines
.write()
.unwrap()
.entry(key)
.or_insert_with(|| Arc::new(P::create(&self.context)))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: P::create() now executes while the pipelines write lock is held, introducing avoidable lock contention during expensive pipeline construction.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/wgpu-executor/src/lib.rs, line 103:

<comment>`P::create()` now executes while the `pipelines` write lock is held, introducing avoidable lock contention during expensive pipeline construction.</comment>

<file context>
@@ -91,17 +91,16 @@ impl WgpuExecutor {
 			.unwrap()
 			.entry(key)
-			.or_insert(built)
+			.or_insert_with(|| Arc::new(P::create(&self.context)))
 			.clone()
 			.downcast::<P>()
</file context>

.clone()
.downcast::<P>()
.expect("TypeId<P> guarantees this downcast")
}

pub async fn request_texture(&self, size: UVec2) -> Arc<wgpu::Texture> {
self.texture_cache.lock().await.request_texture(&self.context.device, size)
pub async fn run_pipeline<P: WgpuPipeline>(&self, args: &P::Args<'_>) -> P::Out {
self.pipeline::<P>().run(self, args).await
}
}

Expand All @@ -122,17 +131,14 @@ impl WgpuExecutor {

let texture_cache = TextureCache::new(TEXTURE_CACHE_SIZE);

let resampler = Resampler::new(&context.device);
let background_compositor = BackgroundCompositor::new(&context.device);
let shader_runtime = ShaderRuntime::new(&context);

Some(Self {
context,
texture_cache: texture_cache.into(),
vello_renderer: vello_renderer.into(),
resampler,
background_compositor,
shader_runtime,
pipelines: RwLock::new(HashMap::new()),
})
}
}
37 changes: 37 additions & 0 deletions node-graph/libraries/wgpu-executor/src/pipeline.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
use std::future::Future;
use std::pin::Pin;

use crate::{WgpuContext, WgpuExecutor};

pub type PipelineFuture<'a, T> = Pin<Box<dyn Future<Output = T> + Send + 'a>>;

pub trait Pipeline: std::any::Any + Send + Sync + Sized {
type Args<'a>;
type Out: Send;

fn create(context: &WgpuContext) -> Self;

fn run<'a>(&'a self, executor: &'a WgpuExecutor, args: &'a Self::Args<'_>) -> PipelineFuture<'a, Self::Out>;
}

pub trait AsyncPipeline: std::any::Any + Send + Sync + Sized {
type Args<'a>;
type Out: Send;

fn create(context: &WgpuContext) -> Self;

fn run<'a>(&'a self, executor: &'a WgpuExecutor, args: &'a Self::Args<'_>) -> impl Future<Output = Self::Out> + Send + 'a;
}

impl<P: AsyncPipeline> Pipeline for P {
type Args<'a> = <P as AsyncPipeline>::Args<'a>;
type Out = <P as AsyncPipeline>::Out;

fn create(context: &WgpuContext) -> Self {
<P as AsyncPipeline>::create(context)
}

fn run<'a>(&'a self, executor: &'a WgpuExecutor, args: &'a Self::Args<'_>) -> PipelineFuture<'a, Self::Out> {
Box::pin(<P as AsyncPipeline>::run(self, executor, args))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: Pipeline dispatch now boxes every async run, adding per-call heap allocation in a hot executor path.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/wgpu-executor/src/pipeline.rs, line 35:

<comment>Pipeline dispatch now boxes every async run, adding per-call heap allocation in a hot executor path.</comment>

<file context>
@@ -0,0 +1,37 @@
+	}
+
+	fn run<'a>(&'a self, executor: &'a WgpuExecutor, args: &'a Self::Args<'_>) -> PipelineFuture<'a, Self::Out> {
+		Box::pin(<P as AsyncPipeline>::run(self, executor, args))
+	}
+}
</file context>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

likely blocked by

error: lifetime bound not satisfied
  --> node-graph/nodes/gstd/src/render_background.rs:17:1
   |
17 | #[node_macro::node(category(""))]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this is a known limitation that will be removed in the future (see issue #100013 <https://github.com/rust-lang/rust/issues/100013> for more information)
   = note: this error originates in the attribute macro `node_macro::node` (in Nightly builds, run with -Z macro-backtrace for more info)

error: lifetime bound not satisfied
  --> node-graph/nodes/gstd/src/render_pixel_preview.rs:13:1
   |
13 | #[node_macro::node(category(""))]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this is a known limitation that will be removed in the future (see issue #100013 <https://github.com/rust-lang/rust/issues/100013> for more information)
   = note: this error originates in the attribute macro `node_macro::node` (in Nightly builds, run with -Z macro-backtrace for more info)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the heads-up — that looks like a blocking compiler limitation.

}
}
Comment on lines +8 to +37
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since WgpuExecutor::run_pipeline always downcasts to the concrete pipeline type P, there is no need for a trait object or boxed futures. We can leverage Rust's support for impl Future in traits to define a single Pipeline trait without any boxing, eliminating the Box::pin allocation on every pipeline execution.

Suggested change
pub trait Pipeline: std::any::Any + Send + Sync + Sized {
type Args<'a>;
type Out: Send;
fn create(context: &WgpuContext) -> Self;
fn run<'a>(&'a self, executor: &'a WgpuExecutor, args: &'a Self::Args<'_>) -> PipelineFuture<'a, Self::Out>;
}
pub trait AsyncPipeline: std::any::Any + Send + Sync + Sized {
type Args<'a>;
type Out: Send;
fn create(context: &WgpuContext) -> Self;
fn run<'a>(&'a self, executor: &'a WgpuExecutor, args: &'a Self::Args<'_>) -> impl Future<Output = Self::Out> + Send + 'a;
}
impl<P: AsyncPipeline> Pipeline for P {
type Args<'a> = <P as AsyncPipeline>::Args<'a>;
type Out = <P as AsyncPipeline>::Out;
fn create(context: &WgpuContext) -> Self {
<P as AsyncPipeline>::create(context)
}
fn run<'a>(&'a self, executor: &'a WgpuExecutor, args: &'a Self::Args<'_>) -> PipelineFuture<'a, Self::Out> {
Box::pin(<P as AsyncPipeline>::run(self, executor, args))
}
}
pub trait Pipeline: std::any::Any + Send + Sync + Sized {
type Args<'a>;
type Out: Send;
fn create(context: &WgpuContext) -> Self;
fn run<'a>(&'a self, executor: &'a WgpuExecutor, args: &'a Self::Args<'_>) -> impl Future<Output = Self::Out> + Send + 'a;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No

error: lifetime bound not satisfied
  --> node-graph/nodes/gstd/src/render_background.rs:17:1
   |
17 | #[node_macro::node(category(""))]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this is a known limitation that will be removed in the future (see issue #100013 <https://github.com/rust-lang/rust/issues/100013> for more information)
   = note: this error originates in the attribute macro `node_macro::node` (in Nightly builds, run with -Z macro-backtrace for more info)

error: lifetime bound not satisfied
  --> node-graph/nodes/gstd/src/render_pixel_preview.rs:13:1
   |
13 | #[node_macro::node(category(""))]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this is a known limitation that will be removed in the future (see issue #100013 <https://github.com/rust-lang/rust/issues/100013> for more information)
   = note: this error originates in the attribute macro `node_macro::node` (in Nightly builds, run with -Z macro-backtrace for more info)

130 changes: 0 additions & 130 deletions node-graph/libraries/wgpu-executor/src/resample.rs

This file was deleted.

1 change: 1 addition & 0 deletions node-graph/nodes/gstd/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ reqwest = { workspace = true }
image = { workspace = true }
base64 = { workspace = true }
wgpu = { workspace = true }
bytemuck = { workspace = true }

# Optional local dependencies
graphene-canvas-utils = { workspace = true, optional = true }
Expand Down
3 changes: 2 additions & 1 deletion node-graph/nodes/gstd/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
pub mod any;
pub mod pixel_preview;
pub mod platform_application_io;
pub mod render_background;
pub mod render_cache;
pub mod render_node;
pub mod render_pixel_preview;
pub mod text;
pub use blending_nodes;
pub use brush_nodes as brush;
Expand Down
71 changes: 0 additions & 71 deletions node-graph/nodes/gstd/src/pixel_preview.rs

This file was deleted.

Loading
Loading