Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRequire immutable, threadsafe render notifier at renderer initialization #1904
Conversation
| frame_config: FrameBuilderConfig, | ||
| recorder: Option<Box<ApiRecordingReceiver>>, | ||
| blob_image_renderer: Option<Box<BlobImageRenderer>>, | ||
| enable_render_on_scroll: bool, | ||
| ) -> RenderBackend { | ||
| ) -> RenderBackend<T> { |
This comment has been minimized.
This comment has been minimized.
| fn new_frame_ready(&mut self); | ||
| fn new_scroll_frame_ready(&mut self, composite_needed: bool); | ||
| fn external_event(&mut self, _evt: ExternalEvent) { | ||
| pub trait RenderNotifier: Send + Clone { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -132,7 +132,7 @@ static NEXT_NAMESPACE_ID: AtomicUsize = ATOMIC_USIZE_INIT; | |||
| /// GPU-friendly work which is then submitted to the renderer in the form of a frame::Frame. | |||
| /// | |||
| /// The render backend operates on its own thread. | |||
| pub struct RenderBackend { | |||
| pub struct RenderBackend<T> { | |||
This comment has been minimized.
This comment has been minimized.
jrmuizel
Oct 21, 2017
Contributor
Is there a reason this needs to be generic instead of using a trait object? Having it generic is going to increase wrench's already bad build times and we don't have any need to inline or devirtualize the notifier.
This comment has been minimized.
This comment has been minimized.
|
Reviewed 7 of 13 files at r1, 6 of 6 files at r2. Comments from Reviewable |
|
@bors-servo r+ Thanks! @staktrace Heads up - this will need an update to Gecko bindings. The render notifier is provided at initialization now and has slightly different requirements. Once this lands, I'll write a Gecko patch for that and post it here. |
|
|
Require immutable, threadsafe render notifier at renderer initialization This addresses one of the top intermittent test failures in Servo and cleans up an awkward part of the renderer API. Instead of storing an optional notifier inside of a shared mutex, we require that the notifier is cloneable and is responsible for its own mutation requirements. This puts the onus on the implementation of a particular notifier to deal with threadsafety concerns, rather than making all consumers pay for it. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1904) <!-- Reviewable:end -->
|
|
|
@glennw thanks for the heads up. Please post the patch to https://bugzil.la/wr-future-update when you have it, thanks! |
|
@staktrace OK, attached a patch to that bug. I'm unfamiliar with bugzilla workflow, but hopefully that should be what you need to update for the notifier API changes (I've tested them locally). |
|
@glennw Perfect, thanks! |
jdm commentedOct 20, 2017
•
edited by larsbergstrom
This addresses one of the top intermittent test failures in Servo and cleans up an awkward part of the renderer API. Instead of storing an optional notifier inside of a shared mutex, we require that the notifier is cloneable and is responsible for its own mutation requirements. This puts the onus on the implementation of a particular notifier to deal with threadsafety concerns, rather than making all consumers pay for it.
This change is