-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[deno] Resolve CTS OOMs in CI #8930
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,8 @@ use deno_core::V8TaskSpawner; | |
| use deno_core::WebIDL; | ||
|
|
||
| use super::device::GPUDevice; | ||
| use super::device::DEVICE_EXTERNAL_MEMORY_SIZE; | ||
| use super::queue::GPUQueue; | ||
| use crate::error::GPUGenericError; | ||
| use crate::webidl::features_to_feature_names; | ||
| use crate::webidl::GPUFeatureName; | ||
|
|
@@ -160,24 +162,46 @@ impl GPUAdapter { | |
| None, | ||
| )?; | ||
|
|
||
| // Associate external memory with the device to encourage V8 to garbage | ||
| // collect devices promptly. | ||
| scope | ||
| .adjust_amount_of_external_allocated_memory(DEVICE_EXTERNAL_MEMORY_SIZE); | ||
|
Comment on lines
+165
to
+168
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, I wonder if we could do that in servo too. |
||
|
|
||
| let spawner = state.borrow::<V8TaskSpawner>().clone(); | ||
| let lost_resolver = v8::PromiseResolver::new(scope).unwrap(); | ||
| let lost_promise = lost_resolver.get_promise(scope); | ||
| let error_handler = Rc::new(super::error::DeviceErrorHandler::new( | ||
| v8::Global::new(scope, lost_resolver), | ||
| spawner, | ||
| )); | ||
|
|
||
| // Create the queue object eagerly so that the wgpu-core queue resource | ||
| // gets cleaned up when the device is garbage collected, even if JS code | ||
| // never accesses the queue property. | ||
| let queue_obj = deno_core::cppgc::make_cppgc_object( | ||
| scope, | ||
| GPUQueue { | ||
| instance: self.instance.clone(), | ||
| error_handler: error_handler.clone(), | ||
| label: descriptor.label.clone(), | ||
| id: queue, | ||
| device, | ||
| }, | ||
| ); | ||
| let queue_obj = v8::Global::new(scope, queue_obj); | ||
|
|
||
| let device = GPUDevice { | ||
| instance: self.instance.clone(), | ||
| id: device, | ||
| queue, | ||
| label: descriptor.label, | ||
| queue_obj: SameObject::new(), | ||
| queue_obj, | ||
| adapter_info: self.info.clone(), | ||
| error_handler: Rc::new(super::error::DeviceErrorHandler::new( | ||
| v8::Global::new(scope, lost_resolver), | ||
| spawner, | ||
| )), | ||
| error_handler, | ||
| adapter: self.id, | ||
| lost_promise: v8::Global::new(scope, lost_promise), | ||
| limits: SameObject::new(), | ||
| features: SameObject::new(), | ||
| weak: std::sync::OnceLock::new(), | ||
| }; | ||
| let device = deno_core::cppgc::make_cppgc_object(scope, device); | ||
| let weak_device = v8::Weak::new(scope, device); | ||
|
|
@@ -190,13 +214,24 @@ impl GPUAdapter { | |
| let null = v8::null(scope); | ||
| set_event_target_data.call(scope, null.into(), &[device.into()]); | ||
|
|
||
| let finalizer = v8::Weak::with_finalizer( | ||
| scope, | ||
| device, | ||
| Box::new(move |isolate| { | ||
| isolate.adjust_amount_of_external_allocated_memory( | ||
| -DEVICE_EXTERNAL_MEMORY_SIZE, | ||
| ); | ||
| }), | ||
| ); | ||
|
|
||
| // Now that the device is fully constructed, give the error handler a | ||
| // weak reference to it. | ||
| // weak reference to it, and store the finalizer weak reference. | ||
| let device = device.cast::<v8::Value>(); | ||
| deno_core::cppgc::try_unwrap_cppgc_object::<GPUDevice>(scope, device) | ||
| .unwrap() | ||
| .error_handler | ||
| .set_device(weak_device); | ||
| let device_ref = | ||
| deno_core::cppgc::try_unwrap_cppgc_object::<GPUDevice>(scope, device) | ||
| .unwrap(); | ||
| device_ref.error_handler.set_device(weak_device); | ||
| device_ref.weak.set(finalizer).unwrap(); | ||
|
|
||
| Ok(v8::Global::new(scope, device)) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,6 @@ use super::bind_group_layout::GPUBindGroupLayout; | |
| use super::buffer::GPUBuffer; | ||
| use super::compute_pipeline::GPUComputePipeline; | ||
| use super::pipeline_layout::GPUPipelineLayout; | ||
| use super::queue::GPUQueue; | ||
| use super::sampler::GPUSampler; | ||
| use super::shader::GPUShaderModule; | ||
| use super::texture::GPUTexture; | ||
|
|
@@ -38,22 +37,29 @@ use crate::shader::GPUCompilationInfo; | |
| use crate::webidl::features_to_feature_names; | ||
| use crate::Instance; | ||
|
|
||
| /// External memory associated with device and queue, to encourage V8 to garbage | ||
| /// collect devices promptly. This seems to be particularly important on DX12 | ||
| /// in CI, where any smaller power of two results in OOM errors. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: This is a crate sync'd between here and Deno upstream, so clarifying whose CI is failing here might be important. |
||
| pub(crate) const DEVICE_EXTERNAL_MEMORY_SIZE: i64 = 1 << 24; // 16 MB | ||
|
|
||
| pub struct GPUDevice { | ||
| pub instance: Instance, | ||
| pub id: wgpu_core::id::DeviceId, | ||
| pub adapter: wgpu_core::id::AdapterId, | ||
| pub queue: wgpu_core::id::QueueId, | ||
|
|
||
| pub label: String, | ||
|
|
||
| pub features: SameObject<GPUSupportedFeatures>, | ||
| pub limits: SameObject<GPUSupportedLimits>, | ||
| pub adapter_info: Rc<SameObject<GPUAdapterInfo>>, | ||
|
|
||
| pub queue_obj: SameObject<GPUQueue>, | ||
| pub queue_obj: v8::Global<v8::Object>, | ||
|
|
||
| pub error_handler: super::error::ErrorHandler, | ||
| pub lost_promise: v8::Global<v8::Promise>, | ||
|
|
||
| // Weak reference to the JS object so we can attach a finalizer. | ||
| pub(crate) weak: std::sync::OnceLock<v8::Weak<v8::Object>>, | ||
| } | ||
|
|
||
| impl Drop for GPUDevice { | ||
|
|
@@ -126,14 +132,8 @@ impl GPUDevice { | |
|
|
||
| #[getter] | ||
| #[global] | ||
| fn queue(&self, scope: &mut v8::HandleScope) -> v8::Global<v8::Object> { | ||
| self.queue_obj.get(scope, |_| GPUQueue { | ||
| id: self.queue, | ||
| device: self.id, | ||
| error_handler: self.error_handler.clone(), | ||
| instance: self.instance.clone(), | ||
| label: self.label.clone(), | ||
| }) | ||
| fn queue(&self) -> v8::Global<v8::Object> { | ||
| self.queue_obj.clone() | ||
| } | ||
|
|
||
| #[fast] | ||
|
|
||
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.
question: Is it feasible to simplify to
…:createRenderPipeline,at_over:*?