From aa48a2c2e3c7699a167d9ffe791f4bb17e9b9f1c Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Sat, 25 Nov 2017 20:28:13 +0100 Subject: [PATCH] Paint worklets: Implement timeout for worklet painter threads When a paint worklet thread takes too long, we would like to move on, since we have a ~16ms budget for rendering @60fps. At the moment, there is no provision in the paintworklet spec to signal such timeouts to the developer. ajeffrey opened an [issue][1] for this, but it got punted to v2 of the spec. Hence we are silently timing out unresponsive paint scripts. The timeout value is chosen to be 10ms by default, and can be overridden by setting the `dom.worklet.timeout_ms` pref. In the absence of such a timeout, the reftest in this commit would fail by timing out the testrunner itself, since the paint script never returns. From my discussions with ajeffrey, this should do until we spec out a way to signal timeouts to the script developer. Since we did not have a better way to trigger a timeout than a busy waiting loop (which would hog one core of the test machine until the timeout was reached), we decided to implement a test only blocking sleep, available to the PaintWorkletGlobalScope. Since `dom.worklet.enabled` enables worklets in general, we also decided to have another pref `dom.worklet.blockingsleep.enabled`, which, in addition to `dom.worklet.enabled`, would be required for the blocking sleep to be available. This fixes #17370. [1]: https://github.com/w3c/css-houdini-drafts/issues/507 --- components/layout/display_list_builder.rs | 27 ++-- components/layout_thread/lib.rs | 4 +- .../script/dom/paintworkletglobalscope.rs | 130 ++++++++++-------- .../webidls/PaintWorkletGlobalScope.webidl | 4 + components/script_traits/lib.rs | 2 +- tests/wpt/mozilla/meta/MANIFEST.json | 34 +++++ .../worklets/test_paint_worklet_timeout.html | 31 +++++ .../worklets/test_paint_worklet_timeout.js | 7 + .../test_paint_worklet_timeout_ref.html | 20 +++ 9 files changed, 189 insertions(+), 70 deletions(-) create mode 100644 tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout.html create mode 100644 tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout.js create mode 100644 tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout_ref.html diff --git a/components/layout/display_list_builder.rs b/components/layout/display_list_builder.rs index 28511b68569..171e56ab9ea 100644 --- a/components/layout/display_list_builder.rs +++ b/components/layout/display_list_builder.rs @@ -1382,7 +1382,7 @@ impl FragmentDisplayListBuilding for Fragment { .map(|argument| argument.to_css_string()) .collect(); - let mut draw_result = match state.layout_context.registered_painters.get(&name) { + let draw_result = match state.layout_context.registered_painters.get(&name) { Some(painter) => { debug!("Drawing a paint image {}({},{}).", name, size_in_px.width, size_in_px.height); let properties = painter.properties().iter() @@ -1397,19 +1397,22 @@ impl FragmentDisplayListBuilding for Fragment { }, }; - let webrender_image = WebRenderImageInfo { - width: draw_result.width, - height: draw_result.height, - format: draw_result.format, - key: draw_result.image_key, - }; + if let Ok(draw_result) = draw_result { + let webrender_image = WebRenderImageInfo { + width: draw_result.width, + height: draw_result.height, + format: draw_result.format, + key: draw_result.image_key, + }; - for url in draw_result.missing_image_urls.drain(..) { - debug!("Requesting missing image URL {}.", url); - state.layout_context.get_webrender_image_for_url(self.node, url, UsePlaceholder::No); + for url in draw_result.missing_image_urls.into_iter() { + debug!("Requesting missing image URL {}.", url); + state.layout_context.get_webrender_image_for_url(self.node, url, UsePlaceholder::No); + } + Some(webrender_image) + } else { + None } - - Some(webrender_image) } fn build_display_list_for_background_gradient(&self, diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 8401305036f..b89df5f6fa7 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -100,8 +100,8 @@ use script_layout_interface::rpc::{LayoutRPC, MarginStyleResponse, NodeOverflowR use script_layout_interface::rpc::TextIndexResponse; use script_layout_interface::wrapper_traits::LayoutNode; use script_traits::{ConstellationControlMsg, LayoutControlMsg, LayoutMsg as ConstellationMsg}; +use script_traits::{DrawAPaintImageResult, PaintWorkletError}; use script_traits::{ScrollState, UntrustedNodeAddress}; -use script_traits::DrawAPaintImageResult; use script_traits::Painter; use selectors::Element; use servo_arc::Arc as ServoArc; @@ -1787,7 +1787,7 @@ impl Painter for RegisteredPainterImpl { device_pixel_ratio: TypedScale, properties: Vec<(Atom, String)>, arguments: Vec) - -> DrawAPaintImageResult + -> Result { self.painter.draw_a_paint_image(size, device_pixel_ratio, properties, arguments) } diff --git a/components/script/dom/paintworkletglobalscope.rs b/components/script/dom/paintworkletglobalscope.rs index bef867b5da4..7e2e5f3f825 100644 --- a/components/script/dom/paintworkletglobalscope.rs +++ b/components/script/dom/paintworkletglobalscope.rs @@ -45,9 +45,10 @@ use js::rust::Runtime; use msg::constellation_msg::PipelineId; use net_traits::image::base::PixelFormat; use net_traits::image_cache::ImageCache; -use script_traits::DrawAPaintImageResult; +use script_traits::{DrawAPaintImageResult, PaintWorkletError}; use script_traits::Painter; use servo_atoms::Atom; +use servo_config::prefs::PREFS; use servo_url::ServoUrl; use std::cell::Cell; use std::collections::HashMap; @@ -58,6 +59,8 @@ use std::sync::Arc; use std::sync::Mutex; use std::sync::mpsc; use std::sync::mpsc::Sender; +use std::thread; +use std::time::Duration; use style_traits::CSSPixel; use style_traits::DevicePixel; use style_traits::SpeculativePainter; @@ -343,7 +346,7 @@ impl PaintWorkletGlobalScope { device_pixel_ratio: TypedScale, properties: Vec<(Atom, String)>, arguments: Vec) - -> DrawAPaintImageResult { + -> Result { let name = self.name.clone(); let (sender, receiver) = mpsc::channel(); let task = PaintWorkletTask::DrawAPaintImage(name, @@ -354,7 +357,14 @@ impl PaintWorkletGlobalScope { sender); self.executor.lock().expect("Locking a painter.") .schedule_a_worklet_task(WorkletTask::Paint(task)); - receiver.recv().expect("Worklet thread died?") + + let timeout = PREFS.get("dom.worklet.timeout_ms") + .as_u64() + .unwrap_or(10u64); + + let timeout_duration = Duration::from_millis(timeout); + receiver.recv_timeout(timeout_duration) + .map_err(|e| PaintWorkletError::from(e)) } } Box::new(WorkletPainter { @@ -364,6 +374,60 @@ impl PaintWorkletGlobalScope { } } +/// Tasks which can be peformed by a paint worklet +pub enum PaintWorkletTask { + DrawAPaintImage(Atom, + TypedSize2D, + TypedScale, + Vec<(Atom, String)>, + Vec, + Sender), + SpeculativelyDrawAPaintImage(Atom, + Vec<(Atom, String)>, + Vec), +} + +/// A paint definition +/// +/// This type is dangerous, because it contains uboxed `Heap` values, +/// which can't be moved. +#[derive(JSTraceable, MallocSizeOf)] +#[must_root] +struct PaintDefinition { + class_constructor: Heap, + paint_function: Heap, + constructor_valid_flag: Cell, + context_alpha_flag: bool, + // TODO: this should be a list of CSS syntaxes. + input_arguments_len: usize, + // TODO: the spec calls for fresh rendering contexts each time a paint image is drawn, + // but to avoid having the primary worklet thread create a new renering context, + // we recycle them. + context: Dom, +} + +impl PaintDefinition { + fn new(class_constructor: HandleValue, + paint_function: HandleValue, + alpha: bool, + input_arguments_len: usize, + context: &PaintRenderingContext2D) + -> Box + { + let result = Box::new(PaintDefinition { + class_constructor: Heap::default(), + paint_function: Heap::default(), + constructor_valid_flag: Cell::new(true), + context_alpha_flag: alpha, + input_arguments_len: input_arguments_len, + context: Dom::from_ref(context), + }); + result.class_constructor.set(class_constructor.get()); + result.paint_function.set(paint_function.get()); + result + } +} + impl PaintWorkletGlobalScopeMethods for PaintWorkletGlobalScope { #[allow(unsafe_code)] #[allow(unrooted_must_root)] @@ -445,58 +509,14 @@ impl PaintWorkletGlobalScopeMethods for PaintWorkletGlobalScope { Ok(()) } -} -/// Tasks which can be peformed by a paint worklet -pub enum PaintWorkletTask { - DrawAPaintImage(Atom, - TypedSize2D, - TypedScale, - Vec<(Atom, String)>, - Vec, - Sender), - SpeculativelyDrawAPaintImage(Atom, - Vec<(Atom, String)>, - Vec), -} - -/// A paint definition -/// -/// This type is dangerous, because it contains uboxed `Heap` values, -/// which can't be moved. -#[derive(JSTraceable, MallocSizeOf)] -#[must_root] -struct PaintDefinition { - class_constructor: Heap, - paint_function: Heap, - constructor_valid_flag: Cell, - context_alpha_flag: bool, - // TODO: this should be a list of CSS syntaxes. - input_arguments_len: usize, - // TODO: the spec calls for fresh rendering contexts each time a paint image is drawn, - // but to avoid having the primary worklet thread create a new renering context, - // we recycle them. - context: Dom, -} - -impl PaintDefinition { - fn new(class_constructor: HandleValue, - paint_function: HandleValue, - alpha: bool, - input_arguments_len: usize, - context: &PaintRenderingContext2D) - -> Box - { - let result = Box::new(PaintDefinition { - class_constructor: Heap::default(), - paint_function: Heap::default(), - constructor_valid_flag: Cell::new(true), - context_alpha_flag: alpha, - input_arguments_len: input_arguments_len, - context: Dom::from_ref(context), - }); - result.class_constructor.set(class_constructor.get()); - result.paint_function.set(paint_function.get()); - result + /// This is a blocking sleep function available in the paint worklet + /// global scope behind the dom.worklet.enabled + + /// dom.worklet.blockingsleep.enabled prefs. It is to be used only for + /// testing, e.g., timeouts, where otherwise one would need busy waiting + /// to make sure a certain timeout is triggered. + /// check-tidy: no specs after this line + fn Sleep(&self, ms: u64) { + thread::sleep(Duration::from_millis(ms)); } } diff --git a/components/script/dom/webidls/PaintWorkletGlobalScope.webidl b/components/script/dom/webidls/PaintWorkletGlobalScope.webidl index 5592d48d148..9eadc47ba44 100644 --- a/components/script/dom/webidls/PaintWorkletGlobalScope.webidl +++ b/components/script/dom/webidls/PaintWorkletGlobalScope.webidl @@ -6,4 +6,8 @@ [Global=(Worklet,PaintWorklet), Pref="dom.worklet.enabled", Exposed=PaintWorklet] interface PaintWorkletGlobalScope : WorkletGlobalScope { [Throws] void registerPaint(DOMString name, VoidFunction paintCtor); + // This function is to be used only for testing, and should not be + // accessible outside of that use. + [Pref="dom.worklet.blockingsleep.enabled"] + void sleep(unsigned long long ms); }; diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs index 9048f689fb9..3159de3a5c2 100644 --- a/components/script_traits/lib.rs +++ b/components/script_traits/lib.rs @@ -869,7 +869,7 @@ pub trait Painter: SpeculativePainter { zoom: TypedScale, properties: Vec<(Atom, String)>, arguments: Vec) - -> DrawAPaintImageResult; + -> Result; } impl fmt::Debug for Painter { diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 5d183740117..c7192ca228c 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -7804,6 +7804,18 @@ ], {} ] + ], + "mozilla/worklets/test_paint_worklet_timeout.html": [ + [ + "/_mozilla/mozilla/worklets/test_paint_worklet_timeout.html", + [ + [ + "/_mozilla/mozilla/worklets/test_paint_worklet_timeout_ref.html", + "==" + ] + ], + {} + ] ] }, "reftest_node": { @@ -12701,6 +12713,16 @@ {} ] ], + "mozilla/worklets/test_paint_worklet_timeout.js": [ + [ + {} + ] + ], + "mozilla/worklets/test_paint_worklet_timeout_ref.html": [ + [ + {} + ] + ], "mozilla/worklets/test_worklet.js": [ [ {} @@ -72432,6 +72454,18 @@ "7c92e508e08d7d146354a5be7fa256ccbd465dde", "support" ], + "mozilla/worklets/test_paint_worklet_timeout.html": [ + "dde3d2d6359d39282cf8dfdfabebed735c7815a8", + "reftest" + ], + "mozilla/worklets/test_paint_worklet_timeout.js": [ + "83cdbd59c905de898b72ba47d3ae10bb95d76301", + "support" + ], + "mozilla/worklets/test_paint_worklet_timeout_ref.html": [ + "193529394981b09b56404678124f6b08ff230a74", + "support" + ], "mozilla/worklets/test_worklet.html": [ "fe9c93a5307c616f878b6623155e1b04c86dd994", "testharness" diff --git a/tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout.html b/tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout.html new file mode 100644 index 00000000000..fd4625b8874 --- /dev/null +++ b/tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout.html @@ -0,0 +1,31 @@ + + + + + + reftest001 + + + + + + + + + + + + + + diff --git a/tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout.js b/tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout.js new file mode 100644 index 00000000000..46e9756cf66 --- /dev/null +++ b/tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout.js @@ -0,0 +1,7 @@ +registerPaint("testgreen", class { + paint(ctx, size) { + ctx.fillStyle = 'green'; + ctx.fillRect(0, 0, size.width, size.height); + sleep(30); // too long for a paintworklet to init + } +}); diff --git a/tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout_ref.html b/tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout_ref.html new file mode 100644 index 00000000000..065d0fad6ff --- /dev/null +++ b/tests/wpt/mozilla/tests/mozilla/worklets/test_paint_worklet_timeout_ref.html @@ -0,0 +1,20 @@ + + + + + + reftest001 + + + + + + + + + +