Skip to content
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

Reduce doc.render() calls if possible #2994

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Reduce doc.render() calls if possible

  • Loading branch information
sotaroikeda committed Aug 29, 2018
commit 1fb5823cca58145f17db251b9067a41ff0040a02
@@ -86,6 +86,8 @@ struct Document {
// The scene with the latest transactions applied, not necessarily built yet.
// what we will send to the scene builder.
pending: SceneData,
// Calll render() on next generate_frame.

This comment has been minimized.

@kvark

kvark Aug 31, 2018

Member

nit: "Calll"

render_on_generate_frame: bool,

view: DocumentView,

@@ -143,6 +145,7 @@ impl Document {
scene: Scene::new(),
removed_pipelines: Vec::new(),
},
render_on_generate_frame: false,
view: DocumentView {
window_size,
inner_rect: DeviceUintRect::new(DeviceUintPoint::zero(), window_size),
@@ -204,10 +207,15 @@ impl Document {
}
};

let scrolled = self.scroll_nearest_scrolling_ancestor(delta, node_index);

let should_render =
should_render &&
self.scroll_nearest_scrolling_ancestor(delta, node_index) &&
scrolled &&
self.render_on_scroll == Some(true);

self.render_on_generate_frame |= scrolled && !should_render;

This comment has been minimized.

@kvark

kvark Aug 31, 2018

Member

could you explain the logic here please?

This comment has been minimized.

@mattwoodrow

mattwoodrow Sep 6, 2018

Contributor

The comment just above this about double borrowing is probably contributing to this being more complex than it should be.

That said, I think we could rename the earlier usage of should_render to found_scroll_node, and then make this code look like:

let mut should_render = false;
if (found_scroll_node && self.scroll_nearest_scrolling_ancestor(delta, node_index) {
  if (self.render_on_scroll == Some(true)) {
    should_render = true;
  } else {
    self.render_on_generate_frame = true;
  }
}

This comment has been minimized.

@kvark

kvark Sep 6, 2018

Member

(heads up - I edited your code a bit)
the suggestion sounds good to me

This comment has been minimized.

@sotaroikeda

sotaroikeda Sep 10, 2018

Author Contributor

Thanks @mattwoodrow, it is what I intended!


DocumentOps {
scroll: true,
render: should_render,
@@ -229,13 +237,18 @@ impl Document {
}
FrameMsg::SetPan(pan) => {
self.view.pan = pan;
self.render_on_generate_frame = true;
DocumentOps::nop()
}
FrameMsg::ScrollNodeWithId(origin, id, clamp) => {
profile_scope!("ScrollNodeWithScrollId");

let should_render = self.scroll_node(origin, id, clamp)
&& self.render_on_scroll == Some(true);
let scrolled = self.scroll_node(origin, id, clamp);

let should_render = scrolled &&
self.render_on_scroll == Some(true);

self.render_on_generate_frame |= scrolled && !should_render;

DocumentOps {
scroll: true,
@@ -583,10 +596,12 @@ impl RenderBackend {
}
SceneMsg::SetPageZoom(factor) => {
doc.view.page_zoom_factor = factor.get();
doc.render_on_generate_frame = true;
DocumentOps::nop()
}
SceneMsg::SetPinchZoom(factor) => {
doc.view.pinch_zoom_factor = factor.get();
doc.render_on_generate_frame = true;
DocumentOps::nop()
}
SceneMsg::SetWindowParameters {
@@ -597,6 +612,7 @@ impl RenderBackend {
doc.view.window_size = window_size;
doc.view.inner_rect = inner_rect;
doc.view.device_pixel_ratio = device_pixel_ratio;
doc.render_on_generate_frame = true;
DocumentOps::nop()
}
SceneMsg::SetDisplayList {
@@ -702,6 +718,13 @@ impl RenderBackend {
self.last_scene_id
}

pub fn set_render_on_generate_frame(&mut self) {

This comment has been minimized.

@NiLSPACE

NiLSPACE Aug 29, 2018

As a complete outsider this seems confusing to me. If a method starts with set I assume I can change it to what I specify, but it always sets it to true. Wouldn't enable_render_on_generate_frame make more sense?

This comment has been minimized.

@sotaroikeda

sotaroikeda Sep 10, 2018

Author Contributor

Thanks for the advice:) Yes, it makes more sense!

// Set render_on_generate_frame for any existing documents.
for (_, doc) in &mut self.documents {
doc.render_on_generate_frame = true;
}
}

pub fn run(&mut self, mut profile_counters: BackendProfileCounters) {
let mut frame_counter: u32 = 0;
let mut keep_going = true;
@@ -829,6 +852,19 @@ impl RenderBackend {
profile_counters: &mut BackendProfileCounters,
frame_counter: &mut u32,
) -> bool {

// Set render_on_generate_frame for any existing documents
// if message includes resource_cache update.
match msg {
ApiMsg::UpdateResources(_) |
ApiMsg::ClearNamespace(_) |
ApiMsg::MemoryPressure |
ApiMsg::DebugCommand(_) => {
self.set_render_on_generate_frame();
}
_ => {}
}

match msg {
ApiMsg::WakeUp => {}
ApiMsg::WakeSceneBuilder => {
@@ -1042,6 +1078,12 @@ impl RenderBackend {
return;
}

if !transaction_msg.resource_updates.is_empty() {
// Set render_on_generate_frame for any existing documents
// if resource_updates is not empty, since the updates could affects to another documents.

This comment has been minimized.

@kvark

kvark Aug 31, 2018

Member

"could affects to another documents" -> "could affect other documents"?

This comment has been minimized.

@kvark

kvark Aug 31, 2018

Member

Why do we draw the connection between resource updates and frame generation here?

This comment has been minimized.

@mattwoodrow

mattwoodrow Sep 6, 2018

Contributor

It's not changing whether we generate a frame, it's just changing if we call render() at that time.

Before this patch we unconditionally called render() before generating a frame, now we only do so if something relevant has changed that would a new render. I assume that a resource update is one of those things, but not 100% sure.

This comment has been minimized.

@kvark

kvark Sep 6, 2018

Member

do you mean "called render() after generating a frame"?

This comment has been minimized.

@mattwoodrow

mattwoodrow Sep 6, 2018

Contributor

We might be confused with terminology. My reading of update_document is that if generate_frame is true, then we set op.render=true, do the render(), and then call new_frame_ready at the end to actually generate the frame.

This patch makes the generate_frame -> op.render bit only happen if it really needs to.

This comment has been minimized.

@sotaroikeda

sotaroikeda Sep 10, 2018

Author Contributor

On windows, we do not get render() for updating the video frame by Bug 1477608 if video frame format and sizes are same. Then the content does not have apz change nor animation nor display list update, we do not need to call render() during generating frame.

self.set_render_on_generate_frame();
}

self.resource_cache.post_scene_building_update(
transaction_msg.resource_updates,
&mut profile_counters.resources,
@@ -1054,6 +1096,7 @@ impl RenderBackend {
profile_scope!("build scene");

doc.build_scene(&mut self.resource_cache, scene_id);
doc.render_on_generate_frame = true;
}

// If we have a sampler, get more frame ops from it and add them
@@ -1084,8 +1127,10 @@ impl RenderBackend {
}

if doc.current.scene.root_pipeline_id.is_some() {
op.render = true;
op.composite = true;
if doc.render_on_generate_frame {
op.render = true;
}
}
}

@@ -1097,12 +1142,14 @@ impl RenderBackend {
op.composite = false;
}

debug_assert!(op.render || !op.composite);
debug_assert!(op.render || !op.composite || transaction_msg.generate_frame);

let mut render_time = None;
if op.render && doc.has_pixels() {
profile_scope!("generate frame");

doc.render_on_generate_frame = false;

*frame_counter += 1;

// borrow ck hack for profile_counters
@@ -1141,9 +1188,10 @@ impl RenderBackend {
);
self.result_tx.send(msg).unwrap();
profile_counters.reset();
} else if op.render {
} else if op.render || transaction_msg.generate_frame {
// WR-internal optimization to avoid doing a bunch of render work if
// there's no pixels. We still want to pretend to render and request
// there's no pixels or if the render work is not necessary on generate_frame.
// We still want to pretend to render and request
// a composite to make sure that the callbacks (particularly the
// new_frame_ready callback below) has the right flags.
let msg = ResultMsg::PublishPipelineInfo(doc.updated_pipeline_info());
@@ -1431,6 +1479,7 @@ impl RenderBackend {
scene,
removed_pipelines: Vec::new(),
},
render_on_generate_frame: false,
view,
clip_scroll_tree: ClipScrollTree::new(),
frame_id: FrameId(0),
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.