1453387 - Add network load states to gecko profiler output

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

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).

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add network load status reports to gecko-profiler markers

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

Comment 3

[Markus Stange [:mstange]]

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.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

> > +  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

[Markus Stange [:mstange]]

#include "ProfileBufferEntry.h" should do it, I think

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add network load status reports to gecko-profiler markers

Comment 7

[Markus Stange [:mstange]]

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!

Comment 8

[Markus Stange [:mstange]]

The perf.html privacy text change has been deployed, so this can land now.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: change Network to Load N to group properly in perf-html

these would be merged to land

Comment 10

[Markus Stange [:mstange]]

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.

Comment 11

[Markus Stange [:mstange]]

Er, I meant SprintfLiteral, I think.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7549ebecdf7e
Add network load status reports to gecko-profiler markers r=mstange

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7549ebecdf7e