Closed
Bug 1453387
Opened 7 years ago
Closed 7 years ago
Add network load states to gecko profiler output
Categories
(Core :: Gecko Profiler, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jesup, Assigned: jesup)
References
(Depends on 2 open bugs)
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
Similar to the devtools network view, it would be very nice to be able to view network loads in the Gecko Profiler and perf-html.io
Step one is to record the info in markers.
We need to use a unique ID since we may load the same URL in overlapping loads. To avoid duplicate information, the first marker (on HttpChannel::Init()) has the URI and an ID and a single-point-in-time, and later status reports have only the ID (and time deltas from the previous marker/state for the request).
Assignee | ||
Comment 1•7 years ago
|
||
Next will come a UI for this in perf-html, and verify things look good. the saved profiles seem to have all the info, correctly reported, and I see them in the Marker Chart
Attachment #8967054 -
Flags: review?(mstange)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Comment 3•7 years ago
|
||
Comment on attachment 8967054 [details] [diff] [review]
Add network load status reports to gecko-profiler markers
Review of attachment 8967054 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, but I'd like to take another look once you no longer store the raw nsresult status codes in the profile JSON.
::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +391,5 @@
> proxyResolveFlags, proxyURI, channelId);
> if (NS_FAILED(rv))
> return rv;
>
> + mLastStatusReported = TimeStamp::Now(); // in case we enable the profiler after Init()
This section needs to be wrapped into #ifdef MOZ_GECKO_PROFILER, because profiler_is_active() and profiler_add_marker() are not defined otherwise.
@@ +402,5 @@
> + rv = NS_OK; // ensure it's a fixed value (0)
> + profiler_add_marker("Network",
> + MakeUnique<NetworkMarkerPayload>(static_cast<int64_t>(channelId),
> + PromiseFlatCString(spec).get(),
> + rv, // NS_OK means START
Huh, NS_NET_STATUS_* status values use nsresult as their status type? Weird.
@@ +7733,5 @@
> }
> }
> }
>
> + if (profiler_is_active()) {
This section also needs to be wrapped into #ifdef MOZ_GECKO_PROFILER.
::: tools/profiler/core/ProfilerMarkerPayload.cpp
@@ +134,5 @@
> + UniqueStacks& aUniqueStacks)
> +{
> + StreamCommonProps("Network", aWriter, aProcessStartTime, aUniqueStacks);
> + aWriter.IntProperty("id", mID);
> + aWriter.IntProperty("status", static_cast<int64_t>(mStatus));
Using the raw nsresult values here means that perf.html will need to know what the numbers mean. We've been trying to move away from a state where perf.html needs to contain knowledge about Gecko internals, so I'd be happier if this code mapped the status codes to strings.
If you're worried about string duplication, you can use
aUniqueStacks.mUniqueStrings->WriteElement(aWriter, yourString);
which will automatically output an integer index into the thread's string table.
Attachment #8967054 -
Flags: review?(mstange)
Assignee | ||
Comment 4•7 years ago
|
||
> > + aWriter.IntProperty("status", static_cast<int64_t>(mStatus));
>
> Using the raw nsresult values here means that perf.html will need to know
> what the numbers mean. We've been trying to move away from a state where
> perf.html needs to contain knowledge about Gecko internals, so I'd be
> happier if this code mapped the status codes to strings.
>
> If you're worried about string duplication, you can use
> aUniqueStacks.mUniqueStrings->WriteElement(aWriter, yourString);
> which will automatically output an integer index into the thread's string
> table.
Seems like I'd need to include something to output the "status:" part; this mUniqueStrings-> stuff isn't used in many places. For now I'll use strings; if you can tell me the "right" way to write the de-duped strings I'll change to that.
Comment 5•7 years ago
|
||
#include "ProfileBufferEntry.h" should do it, I think
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8967833 -
Flags: review?(mstange)
Assignee | ||
Updated•7 years ago
|
Attachment #8967054 -
Attachment is obsolete: true
Comment 7•7 years ago
|
||
Comment on attachment 8967833 [details] [diff] [review]
Add network load status reports to gecko-profiler markers
Review of attachment 8967833 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good! Thanks!
Attachment #8967833 -
Flags: review?(mstange) → review+
Comment 8•7 years ago
|
||
The perf.html privacy text change has been deployed, so this can land now.
Assignee | ||
Comment 9•7 years ago
|
||
these would be merged to land
Attachment #8969361 -
Flags: review?(mstange)
Comment 10•7 years ago
|
||
Comment on attachment 8969361 [details] [diff] [review]
change Network to Load N to group properly in perf-html
Review of attachment 8969361 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +403,5 @@
> + // top 32 bits are process id of the load
> + int32_t id = static_cast<int32_t>(channelId & 0xFFFFFFFF);
> + char name[64];
> + snprintf(name, sizeof(name), "Load: %d", id);
> + name[sizeof(name)-1] = '\0'; // paranoia
Please use VsprintfLiteral from Sprintf.h. This will automatically add the zero terminator.
Attachment #8969361 -
Flags: review?(mstange) → review+
Comment 11•7 years ago
|
||
Er, I meant SprintfLiteral, I think.
Comment 12•7 years ago
|
||
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7549ebecdf7e
Add network load status reports to gecko-profiler markers r=mstange
Comment 13•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•