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

Take origin from current window instead of creating a new one in event of reflow #25777

Merged
merged 2 commits into from Feb 19, 2020
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

Take origin from window instead of creating a new one in case of reflow

Everytime a new LayoutContext was created, it created a new origin which
caused endless stream of image loads to occur in case of reflow. The reason
for this was that the existing image, although cached successfully, was not
used because the entry in hashmap did not match because of different(new)
origin.
This is solved by storing the origin of a window in enum ScriptReflow and
used in creating new LayoutContext in case of reflow.
  • Loading branch information
kunalmohan committed Feb 15, 2020
commit a5b43b7df10cc73d8203f7ee0c3a5e1a1bb1d9f5
@@ -140,6 +140,7 @@ impl<'a> LayoutContext<'a> {
state: PendingImageState::Unrequested(url),
node: node.to_untrusted_node_address(),
id: id,
origin: self.origin.clone(),
};
self.pending_images
.as_ref()
@@ -160,6 +161,7 @@ impl<'a> LayoutContext<'a> {
state: PendingImageState::PendingResponse,
node: node.to_untrusted_node_address(),
id: id,
origin: self.origin.clone(),
};
pending_images.lock().unwrap().push(image);
}
@@ -93,7 +93,7 @@ use servo_atoms::Atom;
use servo_config::opts;
use servo_config::pref;
use servo_geometry::MaxRect;
use servo_url::ServoUrl;
use servo_url::{ImmutableOrigin, ServoUrl};
use std::borrow::ToOwned;
use std::cell::{Cell, RefCell};
use std::collections::HashMap;
@@ -644,13 +644,18 @@ impl LayoutThread {
guards: StylesheetGuards<'a>,
script_initiated_layout: bool,
snapshot_map: &'a SnapshotMap,
origin: Option<ImmutableOrigin>,
) -> LayoutContext<'a> {
let thread_local_style_context_creation_data =
ThreadLocalStyleContextCreationInfo::new(self.new_animations_sender.clone());

LayoutContext {
id: self.id,
origin: self.url.origin(),
origin: if let Some(origin) = origin {
origin
} else {
self.url.origin()
},
style_context: SharedStyleContext {
stylist: &self.stylist,
options: GLOBAL_STYLE_DATA.options.clone(),
@@ -1341,6 +1346,8 @@ impl LayoutThread {
Au::from_f32_px(initial_viewport.height),
);

let origin = data.origin.clone();

// Calculate the actual viewport as per DEVICE-ADAPT § 6
// If the entire flow tree is invalid, then it will be reflowed anyhow.
let document_shared_lock = document.style_shared_lock();
@@ -1482,7 +1489,8 @@ impl LayoutThread {
self.stylist.flush(&guards, Some(element), Some(&map));

// Create a layout context for use throughout the following passes.
let mut layout_context = self.build_layout_context(guards.clone(), true, &map);
let mut layout_context =
self.build_layout_context(guards.clone(), true, &map, Some(origin));

let pool;
let (thread_pool, num_threads) = if self.parallel_flag {
@@ -1738,7 +1746,7 @@ impl LayoutThread {
ua_or_user: &ua_or_user_guard,
};
let snapshots = SnapshotMap::new();
let mut layout_context = self.build_layout_context(guards, false, &snapshots);
let mut layout_context = self.build_layout_context(guards, false, &snapshots, None);

This comment has been minimized.

Copy link
@gterzian

gterzian Feb 17, 2020

Member

Here you might be able to receive the origin as part of the LayoutControlMsg::TickAnimations.

It is sent from

let msg = LayoutControlMsg::TickAnimations;

So there you have the Pipeline available, so you could to change the below:

Some(pipeline) => pipeline.layout_chan.send(msg),

into something like

Some(pipeline) => {
    let msg = LayoutControlMsg::TickAnimations(pipeline.load_data.url.origin());
    pipeline.layout_chan.send(msg);
}

This comment has been minimized.

Copy link
@kunalmohan

kunalmohan Feb 17, 2020

Author Collaborator

Oh...I was misunderstanding the purpose and use of LayoutContext. And it was a really stupid one 🤦‍♂️. And, that None was a part of it. I'll correct that now.

Just out of curiosity what is the difference between layout_thread and layout_thread_2020🤔?

This comment has been minimized.

Copy link
@gterzian

gterzian Feb 17, 2020

Member

Note that I'm not quite sure about the purpose of LayoutContext, I just saw the None there and was thinking that you might not need the option.

layout_thread_2020 is the "new version" of layout, so your fix might require to be implemented in both, or maybe on 2020? I don't know, @SimonSapin can probably tell you more.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Feb 17, 2020

Member

Yes, there’s also components/layout_thread_2020/lib.rs which is very similar to components/layout_thread/lib.rs. See https://github.com/servo/servo/wiki/Layout-2020#using for build instructions.

This comment has been minimized.

Copy link
@kunalmohan

kunalmohan Feb 17, 2020

Author Collaborator

Okay. Earlier, I was under the impression that there might be cases where LayoutContext had to be assigned a new origin and was considering the above code to be a part of that.


let invalid_nodes = {
// Perform an abbreviated style recalc that operates without access to the DOM.
@@ -1622,6 +1622,7 @@ impl Window {
document: self.Document().upcast::<Node>().to_trusted_node_address(),
stylesheets_changed,
window_size: self.window_size.get(),
origin: self.origin().immutable().clone(),
reflow_goal,
script_join_chan: join_chan,
dom_count: self.Document().dom_count(),
@@ -23,7 +23,7 @@ use ipc_channel::ipc::IpcSender;
use libc::c_void;
use net_traits::image_cache::PendingImageId;
use script_traits::UntrustedNodeAddress;
use servo_url::ServoUrl;
use servo_url::{ImmutableOrigin, ServoUrl};
use std::ptr::NonNull;
use std::sync::atomic::AtomicIsize;
use style::data::ElementData;
@@ -138,6 +138,7 @@ pub struct PendingImage {
pub state: PendingImageState,
pub node: UntrustedNodeAddress,
pub id: PendingImageId,
pub origin: ImmutableOrigin,
}

pub struct HTMLMediaData {
@@ -18,7 +18,7 @@ use script_traits::{ConstellationControlMsg, LayoutControlMsg, LayoutMsg as Cons
use script_traits::{ScrollState, UntrustedNodeAddress, WindowSizeData};
use servo_arc::Arc as ServoArc;
use servo_atoms::Atom;
use servo_url::ServoUrl;
use servo_url::{ImmutableOrigin, ServoUrl};
use std::sync::atomic::AtomicBool;
use std::sync::Arc;
use style::context::QuirksMode;
@@ -216,6 +216,8 @@ pub struct ScriptReflow {
pub reflow_goal: ReflowGoal,
/// The number of objects in the dom #10110
pub dom_count: u32,
/// The current window origin
pub origin: ImmutableOrigin,
}

pub struct LayoutThreadInit {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.