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

Implement basic Time To First Paint and First Contentful Paint PWMs #17256

Merged
merged 1 commit into from Jul 20, 2017

Conversation

@ferjm
Copy link
Member

commented Jun 9, 2017

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jun 9, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/lib.rs, components/script/dom/bindings/trace.rs, components/profile/time.rs, components/script_layout_interface/Cargo.toml, components/script/Cargo.toml and 6 more
  • @KiChjang: components/script/lib.rs, components/script/dom/bindings/trace.rs, components/script_layout_interface/Cargo.toml, components/script/Cargo.toml, components/script_layout_interface/lib.rs and 3 more
@highfive

This comment has been minimized.

Copy link

commented Jun 9, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@ferjm

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2017

r? @jdm this is just a first attempt to start the work on PWM. There's still some work to do (i.e. to investigate sending the metrics from WebRender, tests... ), but I would really appreciate if you could give me some feedback about the current approach. Thank you!

With the current patch the metrics are shown in the profiler timeline and in the console when running servo with the --print-pwm option.

@ferjm ferjm assigned jdm and unassigned asajeffrey Jun 9, 2017

@jdm
Copy link
Member

left a comment

This is the right idea, but I think we can still simplify the ownership of the paint metrics data.

@@ -704,6 +710,9 @@ impl LayoutThread {
debug_assert!(self.paint_worklet_executor.is_none());
self.paint_worklet_executor = Some(executor);
},
Msg::SetPaintTimeMetrics(paint_time_metrics) => {
self.paint_time_metrics = Some(paint_time_metrics.clone());

This comment has been minimized.

Copy link
@jdm

jdm Jun 16, 2017

Member

Why is the clone necessary here?

// XXX At some point, we may want to set this metric from WebRender itself.
if let Some(ref paint_time_metrics) = self.paint_time_metrics {
paint_time_metrics.maybe_set_first_paint(self.profiler_metadata());
paint_time_metrics.maybe_set_first_content_paint(self.profiler_metadata(), &display_list);

This comment has been minimized.

Copy link
@jdm

jdm Jun 16, 2017

Member

Let's store a local value rather than call profiler_metadata twice. It would be even nicer if we did not have to create a ProfilerMetadata value unless we actually have to set a marker; perhaps we could have a trait that maybe_set_foo accepts which LayoutThread would implement.

let win = box Window {
globalscope:
GlobalScope::new_inherited(
id,
devtools_chan,
devtools_chan.clone(),

This comment has been minimized.

Copy link
@jdm

jdm Jun 16, 2017

Member

This looks unnecessary.

@@ -88,6 +89,9 @@ pub enum Msg {

/// Tells layout that script has added some paint worklet modules.
SetPaintWorkletExecutor(Arc<PaintWorkletExecutor>),

/// Set the paint time metrics recorder.
SetPaintTimeMetrics(Arc<PaintTimeMetrics>),

This comment has been minimized.

Copy link
@jdm

jdm Jun 16, 2017

Member

What if we add this to LayoutThreadFactory::create instead?

self.set_first_content_paint(profiler_metadata);
return;
},
_ => {

This comment has been minimized.

Copy link
@jdm

jdm Jun 16, 2017

Member

_ => (),

@@ -183,6 +184,8 @@ pub struct Window {
history: MutNullableJS<History>,
custom_element_registry: MutNullableJS<CustomElementRegistry>,
performance: MutNullableJS<Performance>,
#[ignore_heap_size_of = "Arc"]
paint_time_metrics: Arc<PaintTimeMetrics>,

This comment has been minimized.

Copy link
@jdm

jdm Jun 16, 2017

Member

It's not clear why we need to store this in Window at all.

This comment has been minimized.

Copy link
@ferjm

ferjm Jul 4, 2017

Author Member

My understanding is that we want independent metrics per Window. Specially since these metrics are supposed to be exposed through the PerformanceObserver API at some point, which is exposed in Window.

This comment has been minimized.

Copy link
@jdm

jdm Jul 4, 2017

Member

At the moment we have a layout thread per window, and we don't expose this data through the DOM at all. I am inclined to keep it as simple as possible while we can.

This comment has been minimized.

Copy link
@ferjm

ferjm Jul 4, 2017

Author Member

Ok, fair enough. I'll remove it from Window until we implement the PerformanceObserver API then.

@ferjm ferjm force-pushed the ferjm:ttfp branch from 76336f4 to c52ee6a Jul 4, 2017

@ferjm ferjm requested a review from jdm Jul 4, 2017

@ferjm

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2017

r? @jdm

@ferjm ferjm force-pushed the ferjm:ttfp branch from c52ee6a to 6d6ed55 Jul 6, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2017

☔️ The latest upstream changes (presumably #17536) made this pull request unmergeable. Please resolve the merge conflicts.

@ferjm ferjm force-pushed the ferjm:ttfp branch from 6d6ed55 to 4b0cde5 Jul 7, 2017

@jdm
Copy link
Member

left a comment

This is better, but I think there are still some ways we can improve the ownership and correctness.

@@ -1643,6 +1659,11 @@ impl LayoutThread {
}
}

impl<'a> ProfilerMetadataFactory for &'a LayoutThread {

This comment has been minimized.

Copy link
@jdm

jdm Jul 10, 2017

Member

This looks like it can be impl ProfilerMetadataFactory for LayoutThread instead.

@@ -512,7 +515,8 @@ impl UnprivilegedPipelineContent {
Some(self.layout_content_process_shutdown_chan),
self.webrender_api_sender,
self.prefs.get("layout.threads").expect("exists").value()
.as_u64().expect("count") as usize);
.as_u64().expect("count") as usize,
paint_time_metrics.clone());

This comment has been minimized.

Copy link
@jdm

jdm Jul 10, 2017

Member

I think this clone is unnecessary.

let win = box Window {
globalscope:
GlobalScope::new_inherited(
id,
devtools_chan,
mem_profiler_chan,
time_profiler_chan,
time_profiler_chan.clone(),

This comment has been minimized.

Copy link
@jdm

jdm Jul 10, 2017

Member

I think this clone is unnecessary now.

@@ -1395,6 +1401,7 @@ impl ScriptThread {
image_cache: self.image_cache.clone(),
content_process_shutdown_chan: content_process_shutdown_chan,
layout_threads: layout_threads,
paint_time_metrics: self.paint_time_metrics.clone(),

This comment has been minimized.

Copy link
@jdm

jdm Jul 10, 2017

Member

It's not clear to me that we want to clone existing metrics here - this is creating a new layout thread for a subframe of this page, which will be loading an entirely separate document. I think we should be creating a new value, and if we still need it later we should store it in InProgressLoad.

@@ -1938,12 +1948,13 @@ impl ScriptThread {
self.control_chan.clone(),
self.scheduler_chan.clone(),
ipc_timer_event_chan,
incomplete.layout_chan,
incomplete.layout_chan.clone(),

This comment has been minimized.

Copy link
@jdm

jdm Jul 10, 2017

Member

Why is this clone necessary?

@@ -510,6 +511,9 @@ pub struct ScriptThread {
/// A list of nodes with in-progress CSS transitions, which roots them for the duration
/// of the transition.
transitioning_nodes: DOMRefCell<Vec<JS<Node>>>,

/// Paint time metrics.
paint_time_metrics: Arc<PaintTimeMetrics>,

This comment has been minimized.

Copy link
@jdm

jdm Jul 10, 2017

Member

It's not clear to me what this represents - ScriptThread is shared between an arbitrary number of similar-origin documents.

@@ -1919,6 +1926,9 @@ impl ScriptThread {
ROUTER.route_ipc_receiver_to_mpsc_sender(ipc_timer_event_port,
self.timer_event_chan.clone());

let navigation_start_precise = precise_time_ns() as f64;
self.paint_time_metrics.set_navigation_start(navigation_start_precise);

This comment has been minimized.

Copy link
@jdm

jdm Jul 10, 2017

Member

Reading this code made me file #17651. The fact that we wait to record this value until the first network response probably makes this PR more complicated than it would be otherwise.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

☔️ The latest upstream changes (presumably #17694) made this pull request unmergeable. Please resolve the merge conflicts.

@ferjm ferjm force-pushed the ferjm:ttfp branch from 4b0cde5 to 1059a43 Jul 20, 2017

@ferjm ferjm force-pushed the ferjm:ttfp branch 2 times, most recently from 9d3fbe1 to e7e4ded Jul 20, 2017

@ferjm ferjm force-pushed the ferjm:ttfp branch from e7e4ded to 892b30e Jul 20, 2017

@ferjm

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

r? @jdm

@jdm

This comment has been minimized.

Copy link
Member

commented Jul 20, 2017

@bors-servo: r+
Nice, this is way more straightforward than it started!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

📌 Commit 892b30e has been approved by jdm

@jdm
jdm approved these changes Jul 20, 2017
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

⌛️ Testing commit 892b30e with merge eba573d...

bors-servo added a commit that referenced this pull request Jul 20, 2017
Auto merge of #17256 - ferjm:ttfp, r=jdm
Implement basic Time To First Paint and First Contentful Paint PWMs

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17256)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

@bors-servo bors-servo merged commit 892b30e into servo:master Jul 20, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@ferjm ferjm deleted the ferjm:ttfp branch Jul 21, 2017

@ferjm ferjm referenced this pull request Aug 9, 2017
2 of 3 tasks complete
bors-servo added a commit that referenced this pull request Aug 16, 2017
Auto merge of #18028 - ferjm:performance.timeline, r=jdm
Performance Timeline API

[Performance Timeline API](https://www.w3.org/TR/performance-timeline-2/) implementation.

This API is required to allow DOM access to the [Paint Timing API](https://wicg.github.io/paint-timing/#example) metrics implemented in #17256. Unfortunately, I couldn't test it properly, as its usage depends on other APIs like the Paint Timing, User Timing, Resource Timing or Server Timing APIs. I'll work in the integration with the Paint Timing API next.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] There are [WPTs](https://github.com/servo/servo/tree/master/tests/wpt/web-platform-tests/performance-timeline) for this API, however they depend on the implementation of the User Timing and the Resource Timing APIs, which I'll hopefully be implementing soon.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18028)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Aug 16, 2017
Auto merge of #18028 - ferjm:performance.timeline, r=jdm
Performance Timeline API

[Performance Timeline API](https://www.w3.org/TR/performance-timeline-2/) implementation.

This API is required to allow DOM access to the [Paint Timing API](https://wicg.github.io/paint-timing/#example) metrics implemented in #17256. Unfortunately, I couldn't test it properly, as its usage depends on other APIs like the Paint Timing, User Timing, Resource Timing or Server Timing APIs. I'll work in the integration with the Paint Timing API next.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] There are [WPTs](https://github.com/servo/servo/tree/master/tests/wpt/web-platform-tests/performance-timeline) for this API, however they depend on the implementation of the User Timing and the Resource Timing APIs, which I'll hopefully be implementing soon.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18028)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Aug 16, 2017
Auto merge of #18028 - ferjm:performance.timeline, r=jdm
Performance Timeline API

[Performance Timeline API](https://www.w3.org/TR/performance-timeline-2/) implementation.

This API is required to allow DOM access to the [Paint Timing API](https://wicg.github.io/paint-timing/#example) metrics implemented in #17256. Unfortunately, I couldn't test it properly, as its usage depends on other APIs like the Paint Timing, User Timing, Resource Timing or Server Timing APIs. I'll work in the integration with the Paint Timing API next.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] There are [WPTs](https://github.com/servo/servo/tree/master/tests/wpt/web-platform-tests/performance-timeline) for this API, however they depend on the implementation of the User Timing and the Resource Timing APIs, which I'll hopefully be implementing soon.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18028)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Aug 17, 2017
Auto merge of #18028 - ferjm:performance.timeline, r=jdm
Performance Timeline API

[Performance Timeline API](https://www.w3.org/TR/performance-timeline-2/) implementation.

This API is required to allow DOM access to the [Paint Timing API](https://wicg.github.io/paint-timing/#example) metrics implemented in #17256. Unfortunately, I couldn't test it properly, as its usage depends on other APIs like the Paint Timing, User Timing, Resource Timing or Server Timing APIs. I'll work in the integration with the Paint Timing API next.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] There are [WPTs](https://github.com/servo/servo/tree/master/tests/wpt/web-platform-tests/performance-timeline) for this API, however they depend on the implementation of the User Timing and the Resource Timing APIs, which I'll hopefully be implementing soon.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18028)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.