Closed Bug 1353159 Opened 3 years ago Closed 3 years ago

Remove IPC_MESSAGE_SIZE/MESSAGE_MANAGER_MESSAGE_SIZE2 and replace with a single histogram

Categories

(Core :: IPC, enhancement)

enhancement
Not set

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.
Blocks: 1348591
Assignee: nobody → hchang
(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)
(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: nobody → hchang
Attachment #8864428 - Flags: review?(wmccloskey)
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 :)
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+
Counting only >= 4K seems fine.
Flags: needinfo?(wmccloskey)
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
https://hg.mozilla.org/mozilla-central/rev/3740967126ee
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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)
Flags: needinfo?(wmccloskey)
FWIW see bug 1364556.
You need to log in before you can comment on or make changes to this bug.