Closed Bug 1322553 Opened 3 years ago Closed 3 years ago

Add markers for synchronous IPC

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Harald, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Sometimes there's no backtrace available, but you still want to denote
an interesting event.
Attachment #8823729 - Flags: review?(mstange)
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.
Attachment #8823731 - Flags: review?(mstange)
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.
Attachment #8823732 - Flags: review?(mstange)
Attachment #8823729 - Flags: review?(mstange) → review+
Attachment #8823731 - Flags: review?(mstange) → review+
(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).
Blocks: 1329161
New patch; the generated code looks like:

    bool sendok__;
    {
        GeckoProfilerTracingRAII syncIPCTracer(
                "IPC",
                "PJavaScript::Msg_PreventExtensions");
        sendok__ = (GetIPCChannel())->Send(msg__, (&(reply__)));
    }
    if ((!(sendok__))) {
        return false;
    }
Attachment #8824458 - Flags: review?(mstange)
Attachment #8823732 - Attachment is obsolete: true
Attachment #8823732 - Flags: review?(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.
Attachment #8824458 - Flags: review?(mstange) → review+
Pushed by nfroyd@mozilla.com:
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
Depends on: 1329467
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.
Depends on: 1329634
Assignee: nobody → nfroyd
Depends on: 1349856
You need to log in before you can comment on or make changes to this bug.