added time to interactive metrics #18670

avadacatavra · 2017-09-29T00:22:32Z

Added time to interactive metrics and refactored metrics/lib

I need to write tests, but wanted to submit the PR for review

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-09-29T00:22:39Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/document.rs, components/profile/time.rs, components/profile_traits/lib.rs, components/profile_traits/lib.rs, components/script_traits/lib.rs and 8 more
@KiChjang: components/script/dom/document.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/script_thread.rs, components/script/dom/window.rs and 1 more

highfive · 2017-09-29T00:22:40Z

Warning

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

bors-servo · 2017-09-29T01:16:27Z

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

jdm · 2017-09-29T08:23:37Z

It looks like there are a bunch of unrelated changes in Cargo.lock. Did you run a cargo update operation? It might be best to revert all the changes to the file, then build and commit the resulting file.

avadacatavra · 2017-09-29T15:46:52Z

@jdm no clue what I did to Cargo.lock -- I'll fix that

jdm · 2017-10-02T16:36:49Z

./mach test-unit is failing:

error[E0583]: file not found for module `interactive_time`

  --> /home/travis/build/servo/servo/tests/unit/metrics/lib.rs:17:5

   |

17 | mod interactive_time;

   |     ^^^^^^^^^^^^^^^^

   |

   = help: name the file either interactive_time.rs or interactive_time/mod.rs inside the directory "/home/travis/build/servo/servo/tests/unit/metrics"

error: aborting due to previous error

error: Could not compile `metrics_tests`.

warning: build failed, waiting for other jobs to finish...

error: build failed

jdm

Good start, but there are some fundamental issues with the logic for tracking TTI that we need to address.

jdm · 2017-10-09T09:51:39Z

Cargo.toml

-# debug = true
-# lto = false
+debug = true
+lto = false


Revert these changes, please.

jdm · 2017-10-09T09:51:39Z

components/layout_thread/lib.rs

@@ -757,7 +757,7 @@ impl LayoutThread {
                return false
            },
            Msg::SetNavigationStart(time) => {
-                self.paint_time_metrics.set_navigation_start(time);
+                (&self.paint_time_metrics).set_navigation_start(time);


Why is this change necessary?

I made a trait ProgressiveWebMetric with function set_navigation_start, then ended up implementing it on &PaintTimeMetrics. I think I need to change the implementation to impl on PaintTimeMetrics and then make it so that you can call it on a ref

jdm · 2017-10-09T09:51:39Z

components/profile_traits/time.rs

@@ -28,6 +29,12 @@ impl ProfilerChan {
    }
 }

+impl HeapSizeOf for ProfilerChan {


Rather than doing this implementation and adding the heapsize dependency, we can use #[ignore_heap_size_of = "can't measure channels"] at the use site instead.

jdm · 2017-10-09T09:51:39Z

components/script/dom/document.rs

 }

+unsafe_no_jsmanaged_fields!(InteractiveMetrics);


Let's move this to script/dom/bindings/trace.rs.

jdm · 2017-10-09T09:51:39Z

components/script/dom/document.rs

+        // debug!("Adding named element to document {:p}: {:p} id={}",
+               // self,
+               // element,
+               // id);


Revert these changes please.

jdm · 2017-10-09T09:51:39Z

components/script/script_thread.rs

+            ReportCSSError(id, ..) => Some(id),
+            Reload(id, ..) => Some(id),
+            WebVREvents(id, ..) => Some(id),
+            _ => None,


We should avoid wildcard branches.

jdm · 2017-10-09T09:51:39Z

components/script/script_thread.rs

+    fn is_cancelled(&self) -> bool { false }
+    fn name(&self) -> &'static str { "generic runnable" }
+    fn handler(self: Box<Self>) {}
+    fn pipeline(&self) -> Option<PipelineId> { None }


I think we probably want to migrate the pipeline method to the script::task::TaskOnce trait.

jdm · 2017-10-09T09:51:39Z

components/script/script_thread.rs

@@ -673,6 +825,7 @@ impl ScriptThread {
        });
    }

+    //FIXME pipeline id


trailing fixme that i missed :/ sorry

jdm · 2017-10-09T09:51:39Z

components/script/script_thread.rs

@@ -909,6 +1065,8 @@ impl ScriptThread {
            self.handle_event(id, ResizeEvent(size, size_type));
        }

+        // TODO avada can we replace sequential with task times


I'm not sure what this means.

your other comments make that note moot

jdm · 2017-10-09T09:51:39Z

components/metrics/lib.rs

+pub struct InteractiveWindow {
+    start: u64,
+    instant: Instant,
+    all_interactive: bool,


If we load document A, and then after N seconds (but before we would have recorded a TTI metric for it) it adds iframe B, we will now be measuring the TTI for both documents from the start of the window for B, right? We need to have separate window measurements for each document to avoid this, as far as I can tell.

jdm · 2017-10-16T13:14:56Z

components/metrics/lib.rs

@@ -96,7 +93,7 @@ pub fn set_metric<U: ProgressiveWebMetric>(
 }

 // https://github.com/GoogleChrome/lighthouse/issues/27
-#[derive(HeapSizeOf)]
+#[ignore_heap_size_of = "can't measure channels"]


This doesn't work. We should leave the derive, but add #[ignore_heap_size_of = "can't measure channels"] on the time_profiler_chan member instead.

jdm · 2017-10-16T13:14:56Z

components/script/dom/bindings/trace.rs

+        CallValueTracer(
+            tracer,
+            val.ptr.get() as *mut _,
+            GCTraceKindToAscii(val.get().trace_kind()),


The automatic rustfmt of this file makes it harder to review changes :/

avadacatavra · 2017-10-24T21:35:06Z

@bors-servo retry

jdm · 2017-10-24T22:10:20Z

@bors-servo: r+

bors-servo · 2017-10-24T22:10:20Z

📌 Commit 6b70ef7 has been approved by jdm


        Auto merge of #18670 - avadacatavra:interactive-metrics, r=jdm

added time to interactive metrics  Added time to interactive metrics and refactored metrics/lib I need to write tests, but wanted to submit the PR for review ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18670)

bors-servo · 2017-10-25T00:41:12Z

⌛ Testing commit 6b70ef7 with merge 6c2f5b7...

bors-servo · 2017-10-25T01:20:35Z

💔 Test failed - mac-rel-wpt3

jdm · 2017-10-25T02:23:35Z

We have issues where we are trying to root a Trusted object off the the thread that it belongs to (presumably to acquire a pipeline id from its global).

  ▶ CRASH [expected OK] /websockets/Send-data.worker.html
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 17.3.0-devel
  │ assertion failed: LIVE_REFERENCES.with(|ref r|
  │                          {
  │                              let r = r.borrow();
  │                              let live_references = r.as_ref().unwrap();
  │                              self.owner_thread ==
  │                                  (&*live_references) as *const _ as
  │                                      *const libc::c_void
  │                          }) (thread WebWorker for http://web-platform.test:8000/websockets/Send-data.worker.js, at /Users/servo/buildbot/slave/mac-rel-wpt3/build/components/script/dom/bindings/refcounted.rs:180)
  │ 2017-10-24 21:18:52.702 servo[81841:2402443572] Metadata.framework [Error]: couldn't get the client port
  │ stack backtrace:
  │    0:        0x107ec3b2e - backtrace::backtrace::trace::h255aedfedc5a51a4
  │    1:        0x107ec3b6c - backtrace::capture::Backtrace::new::haedf38cc9f3a8a26
  │    2:        0x1066eadf8 - servo::main::_$u7b$$u7b$closure$u7d$$u7d$::h9be21315797ed4ff
  │    3:        0x108714981 - std::panicking::rust_panic_with_hook::he6f3840a88b909da
  │    4:        0x10698bd67 - std::panicking::begin_panic::h5b020f88ef9ed8e8
  │    5:        0x106e491a7 - _$LT$script..dom..bindings..refcounted..Trusted$LT$T$GT$$GT$::root::h5ec36cfe89a78a74
  │    6:        0x106eb11f2 - _$LT$script..dom..dedicatedworkerglobalscope..DedicatedWorkerGlobalScope$u20$as$u20$script..dom..bindings..codegen..Bindings..DedicatedWorkerGlobalScopeBinding..DedicatedWorkerGlobalScopeBinding..DedicatedWorkerGlobalScopeMethods$GT$::PostMessage::h05017917f93e08c9
  │    7:        0x106b25087 - std::panicking::try::do_call::hd365bb5bfd88f865
  │    8:        0x10872172c - __rust_maybe_catch_panic
  │    9:        0x1071cbec8 - script::dom::bindings::codegen::Bindings::DedicatedWorkerGlobalScopeBinding::DedicatedWorkerGlobalScopeBinding::postMessage::h765da7e4c7b04374
  │   10:        0x107fea898 - CallJitMethodOp
  │   11:        0x106e530b3 - script::dom::bindings::utils::generic_call::h38b13b0b45e5c8c0
  │   12:        0x10845644e - js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)
  │   13:        0x10844fc6c - Interpret(JSContext*, js::RunState&)
  │   14:        0x10844332a - js::RunScript(JSContext*, js::RunState&)
  │   15:        0x1084567a2 - js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)
  │   16:        0x108456905 - js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>)
  │   17:        0x1083815bc - js::fun_call(JSContext*, unsigned int, JS::Value*)
  │   18:        0x10845644e - js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)
  │   19:        0x10844fc6c - Interpret(JSContext*, js::RunState&)
  │   20:        0x10844332a - js::RunScript(JSContext*, js::RunState&)
  │   21:        0x1084572e1 - js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*)
  │   22:        0x108457437 - js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*)
  │   23:        0x108324158 - Evaluate(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::StaticScope*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>)
  │   24:        0x108324315 - JS::Evaluate(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, JS::MutableHandle<JS::Value>)
  │   25:        0x107fe79c0 - js::rust::Runtime::evaluate_script::hcc6856294357e0c2
  │   26:        0x107084350 - script::dom::workerglobalscope::WorkerGlobalScope::execute_script::hffd96746716cd969
  │   27:        0x106989ec7 - std::sys_common::backtrace::__rust_begin_short_backtrace::hcb2e09ec5aaf4907
  │   28:        0x106b3bfcd - std::panicking::try::do_call::he07354dc319692f2
  │   29:        0x10872172c - __rust_maybe_catch_panic
  │   30:        0x106c9b7e5 - _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::h0884c2b5741ea087
  │   31:        0x10871375b - std::sys::imp::thread::Thread::new::thread_start::hf09d45f1d3cac3ae
  │   32:     0x7fff8afbb059 - _pthread_body
  │   33:     0x7fff8afbafd6 - _pthread_start
  │ ERROR:servo: assertion failed: LIVE_REFERENCES.with(|ref r|
  │                          {
  │                              let r = r.borrow();
  │                              let live_references = r.as_ref().unwrap();
  │                              self.owner_thread ==
  │                                  (&*live_references) as *const _ as
  │                                      *const libc::c_void
  │                          })
  └ Pipeline failed in hard-fail mode.  Crashing!

jdm · 2017-10-25T02:27:20Z

components/script/dom/dedicatedworkerglobalscope.rs

@@ -358,6 +359,7 @@ impl DedicatedWorkerGlobalScope {
    #[allow(unsafe_code)]
    pub fn forward_error_to_worker_object(&self, error_info: ErrorInfo) {
        let worker = self.worker.borrow().as_ref().unwrap().clone();
+        let pipeline_id = worker.clone().root().global().pipeline_id();


Instead we can use self.upcast::<GlobalScope>().pipeline_id() and that will avoid the panic.


        added pipelines to all task sources

changed task sources to accept pipeline ids

jdm · 2017-10-25T20:53:54Z

@bors-servo: r+

bors-servo · 2017-10-25T20:53:55Z

📌 Commit 52b63de has been approved by jdm

bors-servo · 2017-10-25T20:53:59Z

⌛ Testing commit 52b63de with merge e438b09...


        Auto merge of #18670 - avadacatavra:interactive-metrics, r=jdm

added time to interactive metrics  Added time to interactive metrics and refactored metrics/lib I need to write tests, but wanted to submit the PR for review ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18670)

bors-servo · 2017-10-25T21:56:34Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing e438b09 to master...


        Auto merge of #19218 - avadacatavra:nav-start, r=jdm

added navigation start for interactive metrics  follow up from #18670, fixing #19099 ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #19099 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19218)

avadacatavra requested a review from jdm Sep 29, 2017

highfive assigned pcwalton Sep 29, 2017

highfive added the S-awaiting-review label Sep 29, 2017

highfive added the S-needs-rebase label Sep 29, 2017

jdm assigned jdm and unassigned pcwalton Sep 29, 2017

avadacatavra force-pushed the avadacatavra:interactive-metrics branch from 9091b05 to 92ddc3a Sep 29, 2017

avadacatavra force-pushed the avadacatavra:interactive-metrics branch 5 times, most recently from a38c9f7 to 62c6fe8 Sep 29, 2017

avadacatavra removed the S-needs-rebase label Oct 2, 2017

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 2, 2017

avadacatavra force-pushed the avadacatavra:interactive-metrics branch from 62c6fe8 to dbb5755 Oct 2, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 2, 2017

jdm requested changes Oct 9, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 9, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 14, 2017

jdm reviewed Oct 16, 2017

View changes

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Oct 24, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 25, 2017

jdm reviewed Oct 25, 2017

View changes

added pipelines to all task sources

Loading status checks…

52b63de

changed task sources to accept pipeline ids

avadacatavra force-pushed the avadacatavra:interactive-metrics branch from 6b70ef7 to 52b63de Oct 25, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Oct 25, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 25, 2017

bors-servo merged commit 52b63de into servo:master Oct 25, 2017
3 checks passed

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

highfive removed the S-awaiting-merge label Oct 25, 2017

bors-servo mentioned this pull request Oct 25, 2017

Implement MessagePort and MessageChannel #16622

Closed

avadacatavra mentioned this pull request Nov 2, 2017

Fix TTI logic #19099

Closed

avadacatavra mentioned this pull request Nov 14, 2017

added navigation start for interactive metrics #19218

Merged

3 of 5 tasks complete

servo / servo

added time to interactive metrics #18670

added time to interactive metrics #18670

avadacatavra commented Sep 29, 2017 •

edited by SimonSapin

highfive commented Sep 29, 2017

highfive commented Sep 29, 2017

bors-servo commented Sep 29, 2017

jdm commented Sep 29, 2017

avadacatavra commented Sep 29, 2017

jdm commented Oct 2, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

avadacatavra commented Oct 24, 2017

jdm commented Oct 24, 2017

bors-servo commented Oct 24, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

jdm commented Oct 25, 2017

This comment has been minimized.

jdm commented Oct 25, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

		}

		unsafe_no_jsmanaged_fields!(InteractiveMetrics);

servo / servo

Join GitHub today

added time to interactive metrics #18670

added time to interactive metrics #18670

Conversation

avadacatavra commented Sep 29, 2017 • edited by SimonSapin

highfive commented Sep 29, 2017

highfive commented Sep 29, 2017

bors-servo commented Sep 29, 2017

jdm commented Sep 29, 2017

avadacatavra commented Sep 29, 2017

jdm commented Oct 2, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

avadacatavra commented Oct 24, 2017

jdm commented Oct 24, 2017

bors-servo commented Oct 24, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

jdm commented Oct 25, 2017

This comment has been minimized.

jdm commented Oct 25, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

avadacatavra commented Sep 29, 2017 •

edited by SimonSapin