Closed
Bug 1353159
Opened 8 years ago
Closed 8 years ago
Remove IPC_MESSAGE_SIZE/MESSAGE_MANAGER_MESSAGE_SIZE2 and replace with a single histogram
Categories
(Core :: IPC, enhancement)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: billm, Assigned: hchang)
References
Details
Attachments
(1 file)
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.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → hchang
Assignee | ||
Comment 1•8 years 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 years 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 years ago
|
Assignee: nobody → hchang
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8864428 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 4•8 years 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•8 years 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+
Comment hidden (mozreview-request) |
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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 10•8 years 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•8 years ago
|
Flags: needinfo?(wmccloskey)
Comment 11•8 years ago
|
||
FWIW see bug 1364556.
You need to log in
before you can comment on or make changes to this bug.
Description
•