Expose Navigation Timings as profiler markers for top level content

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Performance Monitoring
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Harald, Assigned: mstange)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Bug 1299117 added first paint as marker. To put it in context we also need markers for the other page load timings.

Each marker should have a payload with the URL that it tracked, to be able to tell multiple loading tabs apart.

Slightly related, this work should be also useful for the Hasal project, as it directly provides page load timings without having to extract them from the page via console.
(Reporter)

Comment 1

2 years ago
I just did mos of this work in a web extension which provides the markers to the profiler by serializing navigation timings using JSON and performance.mark: https://addons.mozilla.org/en-US/firefox/addon/perf-marker/
(Reporter)

Updated

2 years ago
Blocks: 1325161
(Reporter)

Comment 2

a year ago
Created attachment 8843116 [details] [diff] [review]
Strawman for adding markers for navigation timing
(Reporter)

Comment 3

a year ago
Comment on attachment 8843116 [details] [diff] [review]
Strawman for adding markers for navigation timing

There is an option question on how to format markers with more dimensions, like multiple milestones during page load. Does that format work?
Attachment #8843116 - Flags: review?(mstange)
(Assignee)

Comment 4

a year ago
Comment on attachment 8843116 [details] [diff] [review]
Strawman for adding markers for navigation timing

Review of attachment 8843116 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine to me as a starting point. It should probably be approved by somebody who owns this code, though.

The profiler tracing marker format doesn't have a way of associating an end marker with a specific start marker, which means that if you have overlapping page loads, things can get confusing and the frontend might draw marker boxes in the wrong locations. These markers also don't carry any information like URLs.
But I think that's fine to fix once we have support for that in the profiler platform.
Attachment #8843116 - Flags: review?(mstange)
Attachment #8843116 - Flags: review?(bugs)
Attachment #8843116 - Flags: feedback+
Comment on attachment 8843116 [details] [diff] [review]
Strawman for adding markers for navigation timing

Not hot code, so ok.
Attachment #8843116 - Flags: review?(bugs) → review+
Whiteboard: [qf:p3]
(Reporter)

Updated

a year ago
Keywords: checkin-needed
Harald this patch had problems to apply. Can you provide a rebased patch for checkin ? thanks!

Hunk #1 FAILED at 692
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/content.js.rej
patching file browser/base/content/tabbrowser.xml
Hunk #1 FAILED at 4997
Hunk #2 FAILED at 5230
2 out of 2 hunks FAILED -- saving rejects to file browser/base/content/tabbrowser.xml.rej
patching file docshell/base/nsDocShell.cpp
Hunk #1 FAILED at 10193
1 out of 1 hunks FAILED -- saving rejects to file docshell/base/nsDocShell.cpp.rej
patching file dom/base/nsContentUtils.cpp
Hunk #1 FAILED at 4121
1 out of 1 hunks FAILED -- saving rejects to file dom/base/nsContentUtils.cpp.rej
patching file dom/base/nsContentUtils.h
Hunk #1 FAILED at 1250
1 out of 1 hunks FAILED -- saving rejects to file dom/base/nsContentUtils.h.rej
patching file dom/base/test/browser.ini
Hunk #1 FAILED at 0
1 out of 2 hunks FAILED -- saving rejects to file dom/base/test/browser.ini.rej
file dom/base/test/browser_bug1303838.js already exists
1 out of 1 hunks FAILED -- saving rejects to file dom/base/test/browser_bug1303838.js.rej
file dom/base/test/file_bug1303838.html already exists
1 out of 1 hunks FAILED -- saving rejects to file dom/base/test/file_bug1303838.html.rej
file dom/base/test/file_bug1303838_target.html already exists
1 out of 1 hunks FAILED -- saving rejects to file dom/base/test/file_bug1303838_target.html.rej
file dom/base/test/file_bug1303838_with_iframe.html already exists
1 out of 1 hunks FAILED -- saving rejects to file dom/base/test/file_bug1303838_with_iframe.html.rej
patching file dom/notification/Notification.cpp
Hunk #1 FAILED at 316
Hunk #2 FAILED at 1494
2 out of 2 hunks FAILED -- saving rejects to file dom/notification/Notification.cpp.rej
patching file mobile/android/chrome/content/browser.js
Hunk #1 FAILED at 3623
Hunk #2 FAILED at 3739
Hunk #3 FAILED at 4262
3 out of 3 hunks FAILED -- saving rejects to file mobile/android/chrome/content/browser.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1318095
Flags: needinfo?(hkirschner)
Keywords: checkin-needed
(Reporter)

Comment 7

a year ago
:wcpan, I don't have the bandwidth right now to ramp up my local dev environment and get this patch back into a land--ble shape. Could you take a shot at this, as you just added the probes as well?
Flags: needinfo?(hkirschner) → needinfo?(wpan)
Created attachment 8862277 [details] [diff] [review]
0001-Bug-1318095-Strawman-for-adding-markers-for-navigati.patch

Rebased to m-c.
Flags: needinfo?(wpan)
Attachment #8843116 - Attachment is obsolete: true

Comment 9

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3255b0ff3e
Strawman for adding markers for navigation timing. r=smaug
Keywords: checkin-needed

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba3255b0ff3e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.