Closed Bug 1457323 Opened 2 years ago Closed 2 years ago

Link network markers to the process/thread that created them

Categories

(Core :: Gecko Profiler, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: past, Assigned: jesup)

References

Details

Attachments

(1 file, 1 obsolete file)

Network I/O occurs in the parent process, so network markers are displayed when the user selects the Main Thread in perf.html. This increases the visual noise in the marker chart and complicates performance investigations arout a specific site. We should be linking network markers to the content process that created them so that they would be displayed alongside other DOM markers when selecting that process.
It would also be good to include the docshell information, see Bug 1417976. Although we don't really have a plan of action yet on how to use that information.
This adds redirect support, Content process network markers, and reworks the markers to be on start, redirect and end/stop carrying timing info as needed about the phases, instead of on every status update (cutting the load/size).  Also the URL is now part of the marker name, since that's really useful when looking at a marker chart (the complimentary patch for perf-html.io will expand the visible name of markers).
Attachment #8972715 - Flags: review?(mstange)
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Comment on attachment 8972715 [details] [diff] [review]
Add network markers to Content processes, add redirects and improve markers

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

Can you describe which things happen in the parent process and which things happen in the content process, in the commit message? Are we adding any markers in both processes? If so, why?

Many of the #ifdef MOZ_GECKO_PROFILER chunks look very similar. Please try to factor out the common code into a separate method.

::: tools/profiler/core/ProfilerMarkerPayload.cpp
@@ +14,5 @@
>  #include "mozilla/Maybe.h"
>  
>  using namespace mozilla;
>  
> +#define WriteTime(writer, start, time, name) \

Why is this a macro and the method #if 0?

@@ +147,5 @@
>      const char *mName;
>    } NetworkStates[] = {
>      { 0, "STATUS_START" },
> +    { 0xC1F30000, "STATUS_STOP" },
> +    { 0xC1F30001, "STATUS_REDIRECT" },

This is getting a bit out of hand. nsresult was a bad match for load states to begin with, but now that we're adding more states that we don't have nsresult values for, I really think we should make our own enum. The enum should be defined in ProfilerMarkerPayload.h, and there should be a conversion function from nsresult to this enum that can be used in the places where we have an nsresult.
Attachment #8972715 - Flags: review?(mstange) → review-
> Can you describe which things happen in the parent process and which things
> happen in the content process, in the commit message? Are we adding any
> markers in both processes? If so, why?

I will.  I am adding loads in both Content and Master processes. This is intentional; the master will show you all the network traffic occurring, and the content processes will each show the traffic in their process.  Also, you can correlate how much IPC and queuing delay is occurring by matching them up (which can be done in the network view in perf-html/etc).

> 
> Many of the #ifdef MOZ_GECKO_PROFILER chunks look very similar. Please try
> to factor out the common code into a separate method.

Sure; it was on my list - initially we only had 1 or two; now it makes sense to collapse them.  

> > +#define WriteTime(writer, start, time, name) \
> 
> Why is this a macro and the method #if 0?

Debugging cruft; fixed.

> @@ +147,5 @@
> >      const char *mName;
> >    } NetworkStates[] = {
> >      { 0, "STATUS_START" },
> > +    { 0xC1F30000, "STATUS_STOP" },
> > +    { 0xC1F30001, "STATUS_REDIRECT" },
> 
> This is getting a bit out of hand. nsresult was a bad match for load states
> to begin with, but now that we're adding more states that we don't have
> nsresult values for, I really think we should make our own enum. The enum
> should be defined in ProfilerMarkerPayload.h, and there should be a
> conversion function from nsresult to this enum that can be used in the
> places where we have an nsresult.

With this change, I no longer need to mix them with the network nsresult data, so I'm just creating a separate enum.  As I developed the new patch I wasn't sure if I'd still want the individual statuses, but I dumped them along the way as not adding anything and bloating the profile; 90% of the info they gave is in the TimingStruct data.
(In reply to Randell Jesup [:jesup] from comment #4)
> With this change, I no longer need to mix them with the network nsresult
> data, so I'm just creating a separate enum.

I'm not sure I fully understand what you're saying here. Here's the line which I would prefer to disappear and be replaced with a proper enum value: 
    nsresult rv = static_cast<nsresult>(((uint32_t)NS_ERROR_BASE)+1); // arbitrary, means REDIRECT
Markers exist in Master and each Content process has markers for it's own
loads.  Note that there may be a time delay between content and master.
Attachment #8974111 - Flags: review?(mstange)
Attachment #8972715 - Attachment is obsolete: true
Comment on attachment 8974111 [details] [diff] [review]
Add network markers to Content processes, add redirects and improve markers

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

Thanks, much better!

::: tools/profiler/core/platform.cpp
@@ +3343,5 @@
> +                            int64_t aCount,
> +                            const mozilla::net::TimingStruct* aTimings,
> +                            nsIURI* aRedirectURI)
> +{
> +  if (profiler_is_active()) {

could make this an early return

::: tools/profiler/public/GeckoProfiler.h
@@ +537,5 @@
>  void profiler_add_marker_for_thread(int aThreadId,
>                                      const char* aMarkerName,
>                                      mozilla::UniquePtr<ProfilerMarkerPayload> aPayload);
>  
> +enum NetworkLoadType {

Please make this an enum class. GeckoProfiler.h is included in a lot of places, and it's not encapsulated in a namespace, and it'd be good not to pollute the global namespace with LOAD_START etc.
Attachment #8974111 - Flags: review?(mstange) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5df1b51c183
Add network markers to Content processes, add redirects and improve markers r=mstange
Depends on: 1460147
https://hg.mozilla.org/mozilla-central/rev/b5df1b51c183
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.