Skip to content
Open
Show file tree
Hide file tree
Changes from all 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