1322553 - Add markers for synchronous IPC

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[:Harald Kirschner :digitarald]

Came up during the elective. Was deferred in the past by stacks. Would have made :mconley's profiler story easier to identify the bad practice of 

Nice to have would be if there can be additional details about the reason for the IPC message.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add a non-backtrace-taking constructor for GeckoProfilerTracingRAII

Sometimes there's no backtrace available, but you still want to denote
an interesting event.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - use the new constructor of GeckoProfilerTracingRAII

This patch is not strictly relevant to the current bug, but doing the
work here nicely illustrates how the constructor we added in part 1 can
be used in existing parts of the codebase.  It will also encourage people to
DTRT when looking at existing examples for cargo-culting^Winspiration.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add profiler start/end markers for sync IPC

This is gnarly IPDL code, but the generated code is probably easier to
review.  Before when sending a sync message, we had:

    bool sendok__ = (GetIPCChannel())->Send(msg__, (&(reply__)));
    if ((!(sendok__))) {
        return false;
    }

Now, we have:

    bool sendok__;
    {
        GeckoProfilerTracingRAII syncIPCTracer(
                "IPC",
                "SyncMessage");
        sendok__ = (GetIPCChannel())->Send(msg__, (&(reply__)));
    }
    if ((!(sendok__))) {
        return false;
    }

Things I am unsure about:

- Do we want to remove the PROFILER_LABEL for sync IPC?

- Do we want something more informative as the |aInfo| parameter to
  GeckoProfilerTracingRAII, such as the name of the sync message?  Do we want
  to use the protocol name as the category, instead of the generic "IPC"?

Markus, I'm mainly asking you for review since IMHO this is more of a
profiler-related change than an ipdl change, but please feel free to pass the
review along to somebody else.

Comment 4

[Markus Stange [:mstange]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Created attachment 8823732 [details] [diff] [review]
> part 3 - add profiler start/end markers for sync IPC
> 
> This is gnarly IPDL code, but the generated code is probably easier to
> review.  Before when sending a sync message, we had:
> 
>     bool sendok__ = (GetIPCChannel())->Send(msg__, (&(reply__)));
>     if ((!(sendok__))) {
>         return false;
>     }
> 
> Now, we have:
> 
>     bool sendok__;
>     {
>         GeckoProfilerTracingRAII syncIPCTracer(
>                 "IPC",
>                 "SyncMessage");
>         sendok__ = (GetIPCChannel())->Send(msg__, (&(reply__)));
>     }
>     if ((!(sendok__))) {
>         return false;
>     }

Thanks, yes, seeing the generated code makes reviewing this patch much easier.

> Things I am unsure about:
> 
> - Do we want to remove the PROFILER_LABEL for sync IPC?

I don't think so, no. Having the IPC message name in the callstack is still very useful.

> - Do we want something more informative as the |aInfo| parameter to
>   GeckoProfilerTracingRAII, such as the name of the sync message?

Yes, protocol name + message name would be good. Without them the marker isn't all that valuable.

>   Do we want
>   to use the protocol name as the category, instead of the generic "IPC"?

We don't really have a good story of what the categories are for. I think "IPC" is fine for now.

> Markus, I'm mainly asking you for review since IMHO this is more of a
> profiler-related change than an ipdl change, but please feel free to pass the
> review along to somebody else.

I can review it. I've looked at this file before, when I was trying to add the protocol name + message name to tasktracer markers (see bug 1319669 comment 27).

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add profiler start/end markers for sync IPC

New patch; the generated code looks like:

    bool sendok__;
    {
        GeckoProfilerTracingRAII syncIPCTracer(
                "IPC",
                "PJavaScript::Msg_PreventExtensions");
        sendok__ = (GetIPCChannel())->Send(msg__, (&(reply__)));
    }
    if ((!(sendok__))) {
        return false;
    }

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8824458 [details] [diff] [review]
part 3 - add profiler start/end markers for sync IPC

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

Thanks! We don't have the word "sync" inside the marker anymore, but I don't know of a good place to put it, so this is fine.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e3a56c6ed6
part 1 - add a non-backtrace-taking constructor for GeckoProfilerTracingRAII; r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa4b0ffc9d9
part 2 - use the new constructor of GeckoProfilerTracingRAII; r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef46d5e347e2
part 3 - add profiler start/end markers for sync IPC; r=mstange

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30e3a56c6ed6
https://hg.mozilla.org/mozilla-central/rev/2aa4b0ffc9d9
https://hg.mozilla.org/mozilla-central/rev/ef46d5e347e2

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8823729 [details] [diff] [review]
part 1 - add a non-backtrace-taking constructor for GeckoProfilerTracingRAII

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

::: tools/profiler/public/GeckoProfilerImpl.h
@@ +398,5 @@
> +                           MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +    : mCategory(aCategory)
> +    , mInfo(aInfo)
> +  {
> +    MOZ_GUARD_OBJECT_NOTIFIER_INIT;

This is missing a call to profiler_tracing to insert the start marker. Not sure why I didn't catch that.