Skip to content

Add external event notifications in transactions. #3020

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 12, 2018
Merged

Add external event notifications in transactions. #3020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a5c872
Add util::drain_filter.
nical Sep 5, 2018
0e018f1
Add external event notifications in transactions.
nical Sep 5, 2018
503be4a
Use a trait object to handle notifications.
nical Sep 10, 2018
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
12 changes: 12 additions & 0 deletions webrender/src/render_backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use api::{DeviceIntPoint, DevicePixelScale, DeviceUintPoint, DeviceUintRect, Dev
use api::{DocumentId, DocumentLayer, ExternalScrollId, FrameMsg, HitTestFlags, HitTestResult};
use api::{IdNamespace, LayoutPoint, PipelineId, RenderNotifier, SceneMsg, ScrollClamping};
use api::{ScrollLocation, ScrollNodeState, TransactionMsg, ResourceUpdate, ImageKey};
use api::{NotificationRequest, Checkpoint};
use api::channel::{MsgReceiver, Payload};
#[cfg(feature = "capture")]
use api::CaptureBits;
Expand Down Expand Up @@ -44,6 +45,7 @@ use std::u32;
#[cfg(feature = "replay")]
use tiling::Frame;
use time::precise_time_ns;
use util::drain_filter;

#[cfg_attr(feature = "capture", derive(Serialize))]
#[cfg_attr(feature = "replay", derive(Deserialize))]
Expand Down Expand Up @@ -574,6 +576,7 @@ impl RenderBackend {
txn.document_id,
replace(&mut txn.resource_updates, Vec::new()),
replace(&mut txn.frame_ops, Vec::new()),
replace(&mut txn.notifications, Vec::new()),
txn.build_frame,
txn.render_frame,
&mut frame_counter,
Expand Down Expand Up @@ -801,6 +804,7 @@ impl RenderBackend {
resource_updates: transaction_msg.resource_updates,
frame_ops: transaction_msg.frame_ops,
rasterized_blobs: Vec::new(),
notifications: transaction_msg.notifications,
set_root_pipeline: None,
build_frame: transaction_msg.generate_frame,
render_frame: transaction_msg.generate_frame,
Expand Down Expand Up @@ -836,6 +840,7 @@ impl RenderBackend {
txn.document_id,
replace(&mut txn.resource_updates, Vec::new()),
replace(&mut txn.frame_ops, Vec::new()),
replace(&mut txn.notifications, Vec::new()),
txn.build_frame,
txn.render_frame,
frame_counter,
Expand Down Expand Up @@ -872,6 +877,7 @@ impl RenderBackend {
document_id: DocumentId,
resource_updates: Vec<ResourceUpdate>,
mut frame_ops: Vec<FrameMsg>,
mut notifications: Vec<NotificationRequest>,
mut build_frame: bool,
mut render_frame: bool,
frame_counter: &mut u32,
Expand Down Expand Up @@ -981,6 +987,12 @@ impl RenderBackend {
self.result_tx.send(msg).unwrap();
}

drain_filter(
&mut notifications,
|n| { n.when() == Checkpoint::FrameBuilt },
|n| { n.notify(); },
);

// Always forward the transaction to the renderer if a frame was requested,
// otherwise gecko can get into a state where it waits (forever) for the
// transaction to complete before sending new work.
Expand Down
29 changes: 17 additions & 12 deletions webrender/src/resource_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ use std::path::PathBuf;
use std::sync::{Arc, RwLock};
use texture_cache::{TextureCache, TextureCacheHandle, Eviction};
use tiling::SpecialRenderPasses;
use util::drain_filter;

const DEFAULT_TILE_SIZE: TileSize = 512;

Expand Down Expand Up @@ -518,9 +519,8 @@ impl ResourceCache {
updates: &mut Vec<ResourceUpdate>,
profile_counters: &mut ResourceProfileCounters,
) {
let mut new_updates = Vec::with_capacity(updates.len());
for update in mem::replace(updates, Vec::new()) {
match update {
for update in updates.iter() {
match *update {
ResourceUpdate::AddImage(ref img) => {
if let ImageData::Blob(ref blob_data) = img.data {
self.add_blob_image(
Expand All @@ -541,7 +541,7 @@ impl ResourceCache {
);
}
}
ResourceUpdate::SetImageVisibleArea(key, area) => {
ResourceUpdate::SetImageVisibleArea(ref key, ref area) => {
if let Some(template) = self.blob_image_templates.get_mut(&key) {
if let Some(tile_size) = template.tiling {
template.viewport_tiles = Some(compute_tile_range(
Expand All @@ -554,8 +554,17 @@ impl ResourceCache {
}
_ => {}
}
}

match update {
drain_filter(
updates,
|update| match *update {
ResourceUpdate::AddFont(_) |
ResourceUpdate::AddFontInstance(_) => true,
_ => false,
},
// Updates that were moved out of the array:
|update: ResourceUpdate| match update {
ResourceUpdate::AddFont(font) => {
match font {
AddFont::Raw(id, bytes, index) => {
Expand All @@ -567,7 +576,7 @@ impl ResourceCache {
}
}
}
ResourceUpdate::AddFontInstance(mut instance) => {
ResourceUpdate::AddFontInstance(instance) => {
self.add_font_instance(
instance.key,
instance.font_key,
Expand All @@ -577,13 +586,9 @@ impl ResourceCache {
instance.variations,
);
}
other => {
new_updates.push(other);
}
_ => { unreachable!(); }
}
}

*updates = new_updates;
);
}

pub fn set_blob_rasterizer(&mut self, rasterizer: Box<AsyncBlobImageRasterizer>) {
Expand Down
13 changes: 12 additions & 1 deletion webrender/src/scene_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use api::{AsyncBlobImageRasterizer, BlobImageRequest, BlobImageParams, BlobImageResult};
use api::{DocumentId, PipelineId, ApiMsg, FrameMsg, ResourceUpdate, Epoch};
use api::{BuiltDisplayList, ColorF, LayoutSize};
use api::{BuiltDisplayList, ColorF, LayoutSize, NotificationRequest, Checkpoint};
use api::channel::MsgSender;
use frame_builder::{FrameBuilderConfig, FrameBuilder};
use clip_scroll_tree::ClipScrollTree;
Expand All @@ -17,6 +17,7 @@ use scene::Scene;
use std::sync::mpsc::{channel, Receiver, Sender};
use std::mem::replace;
use time::precise_time_ns;
use util::drain_filter;

/// Represents the work associated to a transaction before scene building.
pub struct Transaction {
Expand All @@ -30,6 +31,7 @@ pub struct Transaction {
pub rasterized_blobs: Vec<(BlobImageRequest, BlobImageResult)>,
pub resource_updates: Vec<ResourceUpdate>,
pub frame_ops: Vec<FrameMsg>,
pub notifications: Vec<NotificationRequest>,
pub set_root_pipeline: Option<PipelineId>,
pub build_frame: bool,
pub render_frame: bool,
Expand Down Expand Up @@ -61,6 +63,7 @@ pub struct BuiltTransaction {
pub blob_rasterizer: Option<Box<AsyncBlobImageRasterizer>>,
pub frame_ops: Vec<FrameMsg>,
pub removed_pipelines: Vec<PipelineId>,
pub notifications: Vec<NotificationRequest>,
pub scene_build_start_time: u64,
pub scene_build_end_time: u64,
pub build_frame: bool,
Expand Down Expand Up @@ -251,6 +254,7 @@ impl SceneBuilder {
blob_rasterizer: None,
frame_ops: Vec::new(),
removed_pipelines: Vec::new(),
notifications: Vec::new(),
scene_build_start_time,
scene_build_end_time: precise_time_ns(),
});
Expand Down Expand Up @@ -322,6 +326,12 @@ impl SceneBuilder {
);
rasterized_blobs.append(&mut txn.rasterized_blobs);

drain_filter(
&mut txn.notifications,
|n| { n.when() == Checkpoint::SceneBuilt },
|n| { n.notify(); },
);

Box::new(BuiltTransaction {
document_id: txn.document_id,
build_frame: txn.build_frame || built_scene.is_some(),
Expand All @@ -332,6 +342,7 @@ impl SceneBuilder {
blob_rasterizer: replace(&mut txn.blob_rasterizer, None),
frame_ops: replace(&mut txn.frame_ops, Vec::new()),
removed_pipelines: replace(&mut txn.removed_pipelines, Vec::new()),
notifications: replace(&mut txn.notifications, Vec::new()),
scene_build_start_time,
scene_build_end_time: precise_time_ns(),
})
Expand Down
60 changes: 60 additions & 0 deletions webrender/src/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,3 +508,63 @@ pub fn world_rect_to_device_pixels(
let device_rect = rect * device_pixel_scale;
device_rect.round_out()
}

/// Run the first callback over all elements in the array. If the callback returns true,
/// the element is removed from the array and moved to a second callback.
///
/// This is a simple implementation waiting for Vec::drain_filter to be stable.
/// When that happens, code like:
///
/// let filter = |op| {
/// match *op {
/// Enum::Foo | Enum::Bar => true,
/// Enum::Baz => false,
/// }
/// };
/// drain_filter(
/// &mut ops,
/// filter,
/// |op| {
/// match op {
/// Enum::Foo => { foo(); }
/// Enum::Bar => { bar(); }
/// Enum::Baz => { unreachable!(); }
/// }
/// },
/// );
///
/// Can be rewritten as:
///
/// let filter = |op| {
/// match *op {
/// Enum::Foo | Enum::Bar => true,
/// Enum::Baz => false,
/// }
/// };
/// for op in ops.drain_filter(filter) {
/// match op {
/// Enum::Foo => { foo(); }
/// Enum::Bar => { bar(); }
/// Enum::Baz => { unreachable!(); }
/// }
/// }
///
/// See https://doc.rust-lang.org/std/vec/struct.Vec.html#method.drain_filter
pub fn drain_filter<T, Filter, Action>(
vec: &mut Vec<T>,
mut filter: Filter,
mut action: Action,
)
where
Filter: FnMut(&mut T) -> bool,
Action: FnMut(T)
{
let mut i = 0;
while i != vec.len() {
if filter(&mut vec[i]) {
action(vec.remove(i));
Copy link
Member

@kvark kvark Sep 5, 2018

Choose a reason for hiding this comment

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

This is quadratic asymptotic complexity, or rather O(N*M) where M is the number of filter matches. Can we do better?

Copy link
Contributor Author

@nical nical Sep 5, 2018

Choose a reason for hiding this comment

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

We can but should we? We'll get the the optimized version for free when the std implementation stabilizes and I am certain this will not show up in profiles.
To be honest it also rubs me the wrong way and I'd probably have made more of an effort if we weren't getting the better implementation through std eventually.

Copy link
Member

@kvark kvark Sep 6, 2018

Choose a reason for hiding this comment

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

ok, that's fine by me, assuming there is a limited array size on the input

Copy link
Contributor Author

@nical nical Sep 6, 2018

Choose a reason for hiding this comment

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

Right now the size of resource updates vector is 0 the vast majority of times, 3 while playing a video. occasionally 1 when loading an image, etc.

} else {
i += 1;
}
}
}
Loading