Remove IPC_MESSAGE_SIZE/MESSAGE_MANAGER_MESSAGE_SIZE2 and replace with a single histogram

RESOLVED FIXED in Firefox 55

Status

()

Core
IPC
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: billm, Assigned: hchang)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
These two histograms have expired. However, in bug 1348591, we want to know how big to make the default buffer size for sending and receiving IPC data. We want to balance memory usage (particularly fragmentation) and performance. The new histogram would probably be non-keyed and would count the size of all messages.

Updated

8 months ago
Blocks: 1348591
(Assignee)

Updated

8 months ago
Assignee: nobody → hchang
(Assignee)

Comment 1

8 months ago
(In reply to Bill McCloskey (:billm) from comment #0)
> These two histograms have expired. However, in bug 1348591, we want to know
> how big to make the default buffer size for sending and receiving IPC data.
> We want to balance memory usage (particularly fragmentation) and
> performance. The new histogram would probably be non-keyed and would count
> the size of all messages.

Hi Bill,

I am trying to help investigate bug 1348591 and having the idea of the distribution
of IPC message size seems to be the first thing we need to do. 

If I understand correctly, we'd like to either find the "best" segment capacity
to an templatized capacity for bug 1348591. We now are using 4096 [1] as the 
default capacity for all messages. So, I wonder in this bug, do we need to count
the size of "all" messages or just those messages which size is greater than 4096?
(now we only count the size which is greater than 8192) After all, we can already
afford to 4096 (in terms of memory constraint) and we wouldn't pick a value less
than it eventually.

Or, do you think it's still valuable to have a precise picture of the distribution?

Thanks :) 

[1] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/ipc/chromium/src/base/pickle.cc#38
Assignee: hchang → nobody
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 2

8 months ago
(In reply to Henry Chang [:henry][:hchang] from comment #1)
> (In reply to Bill McCloskey (:billm) from comment #0)
> > These two histograms have expired. However, in bug 1348591, we want to know
> > how big to make the default buffer size for sending and receiving IPC data.
> > We want to balance memory usage (particularly fragmentation) and
> > performance. The new histogram would probably be non-keyed and would count
> > the size of all messages.
> 
> Hi Bill,
> 
> I am trying to help investigate bug 1348591 and having the idea of the
> distribution
> of IPC message size seems to be the first thing we need to do. 
> 
> If I understand correctly, we'd like to either find the "best" segment
> capacity
> to an templatized capacity for bug 1348591. We now are using 4096 [1] as the
  ^^
  should be "or" (sorry for the typo)
(Assignee)

Updated

8 months ago
Assignee: nobody → hchang
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8864428 - Flags: review?(wmccloskey)
(Assignee)

Comment 4

8 months ago
Hi Bill,

I have submitted a patch which only count the message size larger than 4096 first.
Please feel free to drop the review if you think we should still count every single
message size. 

Thanks :)
(Reporter)

Comment 5

7 months ago
mozreview-review
Comment on attachment 8864428 [details]
Bug 1353159 - Use IPC_MESSAGE_SIZE2 to unify the expired IPC_MESSAGE_SIZE and MESSAGE_MANAGER_MESSAGE_SIZE2

https://reviewboard.mozilla.org/r/136114/#review140308

::: dom/base/nsFrameMessageManager.cpp:596
(Diff revision 1)
>    }
>  
>    NS_ConvertUTF16toUTF8 messageName(aMessageName);
>    messageName.StripChars("0123456789");
>  
> -  Telemetry::Accumulate(Telemetry::MESSAGE_MANAGER_MESSAGE_SIZE2, messageName,
> +  Telemetry::Accumulate(Telemetry::IPC_MESSAGE_SIZE2, aDataLength);

You don't need this. The other Accumulate call will count these messages as well. You can remove the const above too.

::: toolkit/components/telemetry/Histograms.json:10693
(Diff revision 1)
> -    "expires_in_version": "55",
> +    "expires_in_version": "60",
>      "kind": "exponential",
>      "high": 8000000,
>      "n_buckets": 50,
> -    "keyed": true,
> -    "description": "Measures the size of IPC messages by message name"
> +    "keyed": false,
> +    "description": "Measures the size (only when greater than 4096 bytes) of IPC messages by message name. This tag integrates IPC_MESSAGE_SIZE and MESSAGE_MANAGER_MESSAGE_SIZE2 which have both expired."

Please change this to:
"Measures the size of all IPC messages sent that are >= 4096 bytes."

The part about the others expiring won't really help anyone reading the code now. You can put that in the commit message.
Attachment #8864428 - Flags: review?(wmccloskey) → review+
(Reporter)

Comment 6

7 months ago
Counting only >= 4K seems fine.
Flags: needinfo?(wmccloskey)
Comment hidden (mozreview-request)

Comment 8

7 months ago
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3740967126ee
Use IPC_MESSAGE_SIZE2 to unify the expired IPC_MESSAGE_SIZE and MESSAGE_MANAGER_MESSAGE_SIZE2 r=billm

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3740967126ee
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 10

7 months ago
I tried to have a look at how the histogram looks like at [1]
but didn't see any incoming data tagged with IPC_MESSAGE_SIZE2.

Even more, I tried to check the IPC_MESSAGE_SIZE in the old builds
but didn't anything, too. 

Does that mean there has never been any message size which is 
greater than 4K/8K ? :(

[1] https://telemetry.mozilla.org
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

7 months ago
Flags: needinfo?(wmccloskey)

Comment 11

7 months ago
FWIW see bug 1364556.
You need to log in before you can comment on or make changes to this bug.