1325526 - Remove IPC message overhead for TaskTracer being disabled

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Thinker Li [:sinker]]

TaskTracer make a 32bytes overhead for each IPC Messages, even it is disabled.  This bug would reduce or remove the overhead to make it more efficient, at least during the time TaskTracer being disabled.

Comment 1

[Thinker Li [:sinker]]

Attachment: Remove IPC message overhead for TaskTracer being disabled

Use a variable size header to remove memory overhead during TaskTracer being disabled.  This patch use a flag in Message::flags to control the size of header.  Headers include variables for TaskTracer only if TaskTracer is enabled, so the size of head grow up on if TaskTracer is built and enabled.

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8821409 [details] [diff] [review]
Remove IPC message overhead for TaskTracer being disabled

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

The following problems need to be addressed.

::: ipc/chromium/src/chrome/common/ipc_message.cc
@@ +48,5 @@
> +    HeaderTaskTracer* _header = static_cast<HeaderTaskTracer*>(header());
> +    GetCurTraceInfo(&_header->source_event_id,
> +                    &_header->parent_task_id,
> +                    &_header->source_event_type);
> +  }

I think there could data corruption if TaskTracer is enabled between the 2 GetOrCreateTraceInfo() calls in the member initialization list (in the MSG_HEADER_SZ macro) and the constructor body.

@@ +82,5 @@
> +    HeaderTaskTracer* _header = static_cast<HeaderTaskTracer*>(header());
> +    GetCurTraceInfo(&_header->source_event_id,
> +                    &_header->parent_task_id,
> +                    &_header->source_event_type);
> +  }

Same here.

::: ipc/chromium/src/chrome/common/ipc_message.h
@@ +237,5 @@
> +#ifdef MOZ_TASK_TRACER
> +    return ((range_end - range_start >= sizeof(Header)) &&
> +            (reinterpret_cast<const Header*>(range_start)->flags &
> +             TASKTRACER_BIT)) ?
> +      sizeof(HeaderTaskTracer) : sizeof(Header);

The logic here is incorrect. When TaskTracer is enabled, and we have a partial read with sizeof(Header) < size <= sizeof(HeaderTaskTracer), this function incorrectly returns sizeof(Header) and confuses Message::MessageSize(). This data shift will then lead to data corruption or ReadSentinel() failure on the receiving end.

@@ +242,5 @@
> +#else
> +    return sizeof(Header);
> +#endif
> +  }
> +

nit: this function doesn't need to be public.

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #2)
> Comment on attachment 8821409 [details] [diff] [review]
> Remove IPC message overhead for TaskTracer being disabled
> 
> Review of attachment 8821409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/chromium/src/chrome/common/ipc_message.h
> @@ +237,5 @@
> > +#ifdef MOZ_TASK_TRACER
> > +    return ((range_end - range_start >= sizeof(Header)) &&
> > +            (reinterpret_cast<const Header*>(range_start)->flags &
> > +             TASKTRACER_BIT)) ?
> > +      sizeof(HeaderTaskTracer) : sizeof(Header);
> 
> The logic here is incorrect. When TaskTracer is enabled, and we have a
> partial read with sizeof(Header) < size <= sizeof(HeaderTaskTracer), this
> function incorrectly returns sizeof(Header) and confuses
> Message::MessageSize(). This data shift will then lead to data corruption or
> ReadSentinel() failure on the receiving end.
> 
Scratch this comment per our offline discussion. This hunk, though tricky, is correct.

Comment 4

[Thinker Li [:sinker]]

Attachment: Remove IPC message overhead for TaskTracer being disabled, v2

Comment 5

[Thinker Li [:sinker]]

Attachment: Remove IPC message overhead for TaskTracer being disabled, v3

r=cyu

Comment 6

[Thinker Li [:sinker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21578aad2ac857df7286f7ee079d400e96cea790

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96af465e0991
Remove IPC message overhead for TaskTracer being disabled. r=cyu

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96af465e0991