BufferList default size for IPC messages maybe too large

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kanru, Assigned: hchang)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [qf:p1][MemShrink])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

2 years ago
This was noticed in bug 1349991 comment 54 and 58. The initial size for IPC messages minus the header was only 36 bytes. This is clearly not enough for some smallish messages but the next allocation will always be 4K.

I think we may want to enlarge the default size or use exact size for fixed size messages.
Whiteboard: [qf] → [qf][MemShrink]
Assignee

Comment 1

2 years ago
As a followup bug for bug 1348591, we might need a mid-term (or long-term) 
IPC message size monitoring to come out the most customized IPC message sizes.
The first step is probably to key IPC_MESSAGE_SIZE2 [1] and log all sizes [1]
so that we can have complete data for analysis.


[1] http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/ipc/glue/MessageChannel.cpp#875
Assignee

Updated

2 years ago
Assignee: nobody → hchang

Updated

2 years ago
Whiteboard: [qf][MemShrink] → [qf:p1][MemShrink]
Assignee

Comment 2

2 years ago
For this particular bug, I wonder if the telemetry data would help since 
the distribution doesn't seem to show as fine-grained as desired.
I don't know if the raw data has been processed or just not shown on
the dashboard.
Reporter

Comment 3

2 years ago
(In reply to Henry Chang [:hchang] from comment #2)
> For this particular bug, I wonder if the telemetry data would help since 
> the distribution doesn't seem to show as fine-grained as desired.
> I don't know if the raw data has been processed or just not shown on
> the dashboard.

The original telemetry collect message size > 4K so it's not useful for this case. We need to create a new telemetry to collect more fine grained usage.
Assignee

Comment 4

2 years ago
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #3)
> (In reply to Henry Chang [:hchang] from comment #2)
> > For this particular bug, I wonder if the telemetry data would help since 
> > the distribution doesn't seem to show as fine-grained as desired.
> > I don't know if the raw data has been processed or just not shown on
> > the dashboard.
> 
> The original telemetry collect message size > 4K so it's not useful for this
> case. We need to create a new telemetry to collect more fine grained usage.

Even if we have new probe, I don't know if we will have helpful distribution.
Take [1] as example, the bucket size is around 2k bytes. Not sure if the bucket
size is tunable. 

[1] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-07-16&keys=__none__!__none__!__none__&max_channel_version=nightly%252F56&measure=IPC_MESSAGE_SIZE2&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-06-12&table=0&trim=1&use_submission_date=0
Assignee

Comment 5

2 years ago
I have done a very initial analysis as shown in [1].
There are around 100 messages which size is always less than 64 bytes.

[1] https://docs.google.com/spreadsheets/d/1p7PoXA7K-sGZWb7AkfBwMyebNHBQCrVFS5JesmuB7cQ/edit#gid=2063339041
Reporter

Comment 6

2 years ago
(In reply to Henry Chang [:hchang] from comment #5)
> I have done a very initial analysis as shown in [1].
> There are around 100 messages which size is always less than 64 bytes.
> 
> [1]
> https://docs.google.com/spreadsheets/d/1p7PoXA7K-
> sGZWb7AkfBwMyebNHBQCrVFS5JesmuB7cQ/edit#gid=2063339041

The document is not viewable. Could you paste it to somewhere like https://pastebin.mozilla.org/ or upload as a attachment?
Assignee

Comment 7

2 years ago
Sorry I forgot to change the permission. Now it's accessible.

This initial analysis is based on around 5-minute trace so it's not very
representative. I'll have a new one today.
Assignee

Comment 8

2 years ago
https://docs.google.com/spreadsheets/d/1CpuO7BnSW9p7PjvRSTBkNUegRVt3-2GZhWw1YuptPnQ

Based on the trace including watching video, reading facebook feeds,
email, sites with massive images.

Frequent (> 1%) small-size messages:

1.  PCompositorBridge::Msg_DidComposite                (~45%),  all < 128 bytes
2.  PVsync::Msg_Notify,                                (~7%),   all < 64  bytes
3.  PBrowser::Msg_RealMouseMoveEvent                   (~3%),   all < 192 bytes
6.  PCompositorBridge::Msg_PTextureConstructor         (~2%),   all < 192 bytes
7.  PTexture::Msg___delete__                           (~2%),   all < 64  bytes
10. PLayerTransaction::Msg_InitReadLocks               (~1%),   98% < 256 bytes
12. PLayerTransaction::Msg_ReleaseLayer                (~1%),   all < 64  bytes
15. PBackground::Msg_PHttpBackgroundChannelConstructor (~1%),   all < 64  bytes
16. PHttpBackgroundChannel::Msg___delete__             (~1%),   all < 64  bytes
18. PHttpChannel::Msg_DeleteSelf                       (~1%),   all < 64  bytes
19. PHttpChannel::Msg___delete__                       (~1%),   all < 64  bytes
20. PHttpBackgroundChannel::Msg_OnStartRequestSent     (~1%),   all < 64  bytes
22. PHttpBackgroundChannel::Msg_OnStopRequest          (~1%),   all < 192 bytes
23. PHttpChannel::Msg_DeletingChannel                  (~1%),   all < 64  bytes
Assignee

Comment 9

2 years ago
(In reply to Henry Chang [:hchang] from comment #8)
> https://docs.google.com/spreadsheets/d/1CpuO7BnSW9p7PjvRSTBkNUegRVt3-
> 2GZhWw1YuptPnQ
> 
> Based on the trace including watching video, reading facebook feeds,
> email, sites with massive images.
> 
> Frequent (> 1%) small-size messages:

The size is grabbed from 

http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/ipc/glue/MessageChannel.cpp#876,1359

so it already includes the header and the payload.
Assignee

Comment 10

2 years ago
Posted patch Bug1380974.patch (obsolete) — Splinter Review
Assignee

Comment 11

2 years ago
Posted patch Bug1380974.patch (obsolete) — Splinter Review
Enlarging the default header capacity may be a better approach.
Attachment #8888154 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8888158 - Flags: review?(kchen)
Reporter

Comment 12

2 years ago
Comment on attachment 8888158 [details] [diff] [review]
Bug1380974.patch

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

::: ipc/chromium/src/base/pickle.cc
@@ +31,5 @@
>  #else
>  // TaskTracer would add extra fields to the header to carry task ID and
>  // other information.
>  // \see class Message::HeaderTaskTracer
>  static const uint32_t kHeaderSegmentCapacity = 128;

If we make the default 256 bytes, we should also adjust the case for TaskTracer. Or we can remove the special case for TaskTracer altogether.

I would like :billm to review this.
Attachment #8888158 - Flags: review?(kchen) → review?(wmccloskey)
Assignee

Comment 13

2 years ago
Posted patch Bug1380974.patch (obsolete) — Splinter Review
Attachment #8888158 - Attachment is obsolete: true
Attachment #8888158 - Flags: review?(wmccloskey)
Attachment #8888229 - Flags: review?(wmccloskey)
I guess I'm not convinced here. It seems like a better solution would be to increase the size for DidComposite and a few other messages in message-metadata.ini, but leave the others alone. Since there are still a lot of messages that are <= 64 bytes, we would be wasting a lot of space by bumping the initial segment to 256 bytes.

If you want to look at this further, I think it would be cool to have a script that would output the number of bytes wasted and the number of allocations needed for a given set of message allocations that you've logged. Then you could adjust the default size and the size for individual messages and try to minimize allocations and wasted bytes.
Assignee

Comment 15

2 years ago
(In reply to Bill McCloskey (:billm) from comment #14)
> I guess I'm not convinced here. It seems like a better solution would be to
> increase the size for DidComposite and a few other messages in
> message-metadata.ini, but leave the others alone. Since there are still a
> lot of messages that are <= 64 bytes, we would be wasting a lot of space by
> bumping the initial segment to 256 bytes.
> 

I guess you mean the first patch I attached attachment 8888154 [details] [diff] [review],
where a few messages are with custom segment size.

> If you want to look at this further, I think it would be cool to have a
> script that would output the number of bytes wasted and the number of
> allocations needed for a given set of message allocations that you've
> logged. Then you could adjust the default size and the size for individual
> messages and try to minimize allocations and wasted bytes.

I was also thinking of the similar thing: to minimize wasted memory and allocations.
However, I haven't figured out a good penalty function to "optimize". 
The reason why I want a penalty function is then we can automatically
generate message-metadata.ini.

Besides, we can further

1) analyze fixed-size messages and variable-size messages respectively.
2) use different "first segment capacity" and "standard capacity" 
   (might only helpful for variable-size messages)

I'll try what I can dig from the trace. Before that, would you mind
reviewing attachment 8888154 [details] [diff] [review] first? Thanks :)
Assignee

Updated

2 years ago
Attachment #8888154 - Attachment is obsolete: false
Attachment #8888154 - Flags: review?(wmccloskey)
Assignee

Updated

2 years ago
Attachment #8888229 - Attachment is obsolete: true
Attachment #8888229 - Flags: review?(wmccloskey)
Comment on attachment 8888154 [details] [diff] [review]
Bug1380974.patch

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

Thanks!
Attachment #8888154 - Flags: review?(wmccloskey) → review+
Reporter

Comment 17

2 years ago
(In reply to Henry Chang [:hchang] from comment #15)
> (In reply to Bill McCloskey (:billm) from comment #14)
> > If you want to look at this further, I think it would be cool to have a
> > script that would output the number of bytes wasted and the number of
> > allocations needed for a given set of message allocations that you've
> > logged. Then you could adjust the default size and the size for individual
> > messages and try to minimize allocations and wasted bytes.
> 
> I was also thinking of the similar thing: to minimize wasted memory and
> allocations.
> However, I haven't figured out a good penalty function to "optimize". 
> The reason why I want a penalty function is then we can automatically
> generate message-metadata.ini.
> 
> Besides, we can further
> 
> 1) analyze fixed-size messages and variable-size messages respectively.
> 2) use different "first segment capacity" and "standard capacity" 
>    (might only helpful for variable-size messages)

Yes, we should aim for generating the size automatically, maybe in another bug. I'm worried that message-metadata.ini may be out of sync with reality because we don't know when message size changes.
Assignee

Comment 18

2 years ago
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #17)
> Yes, we should aim for generating the size automatically, maybe in another
> bug. I'm worried that message-metadata.ini may be out of sync with reality
> because we don't know when message size changes.

Maybe we can get this done with the help of Telemetry alert system:
We probe the "wasted memory" or "allocation count" and telemetry backend
would notify us if there is a spike.
Assignee

Comment 20

2 years ago
Attachment #8888154 - Attachment is obsolete: true
Attachment #8889254 - Flags: review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 21

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c35ec94f3ed
Use custom size for small frequent messages. r=billm
Keywords: checkin-needed

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3c35ec94f3ed
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Reporter

Comment 23

2 years ago
Could we uplift this to 55? It seems rather safe to me.
Flags: needinfo?(hchang)
Assignee

Comment 24

2 years ago
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #23)
> Could we uplift this to 55? It seems rather safe to me.

Sure but we have to uplift bug 1348591 as well. Let me see if there's a
conflict to 55.
Flags: needinfo?(hchang)
Assignee

Comment 25

2 years ago
I've verified bug 1348591 and this one can be directly applied to 55
and run very well. So, Kanru, if you think we should uplift to 55 I
can make a request then.
Reporter

Comment 26

2 years ago
(In reply to Henry Chang [:hchang] from comment #25)
> I've verified bug 1348591 and this one can be directly applied to 55
> and run very well. So, Kanru, if you think we should uplift to 55 I
> can make a request then.

I think uplifting bug 1348591 at this point is a bit scary considering it's only a week before merge. Let's just ride the train then.
You need to log in before you can comment on or make changes to this bug.