Closed Bug 1263235 Opened 4 years ago Closed 4 years ago

Put large arguments last for common messages to reduce internal fragmentation

Categories

(Core :: IPC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
e10s + ---
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: [MemShrink] btpp-active)

Attachments

(4 files)

Currently, IPC::Message grows the buffer as it goes: when a new field is being written in, it takes the larger of (the current size + the size needed for the current field) and (2 times the current size). Any time we hit the second case, we could cause internal fragmentation, wasting memory.

Many messages, such as PHttpChannel::OnTransportAndData(), have a number of small fixed sizes arguments plus one large data buffer argument. By putting the data buffer argument last, we can eliminate the wasteful second case most of the time.

In local testing loading cnn.com, this eliminated almost all of the doubling behavior. (See bug 1253131 for a more complex approach that instead precomputes the expected message size to similarly eliminate doubling.)
"In local testing loading cnn.com, this eliminated almost all of the doubling behavior."
Actually, I should qualify that: it eliminated almost all of the doubling behavior _for messages above 10kb_.
I've converted PBrowser::AsyncMessage, PContent::AsyncMessage, PContentBridge::AsyncMessage, PHttpChannel::OnTransportAndData and PBrowserStream::Write. These are all frequent messages that have an obvious data buffer argument.

There are some PLayerTransaction messages that are also very frequent, but they pass multiple array arguments, so it isn't clear what should be last. They are also not that big.
I also looked at the two PStorage messages mentioned in bug 1263027, but these messages already have the "value" argument last, which seemed like the mostly likely to be large.
This will reduce internal memory fragmentation for the IPC::Message used to make this call.
Attachment #8739641 - Flags: review?(jduell.mcbugs)
This will reduce internal memory fragmentation for the IPC::Message used to make this call.
Attachment #8739642 - Flags: review?(jmathies)
Attachment #8739638 - Flags: review?(bugs) → review+
Comment on attachment 8739640 [details] [diff] [review]
part 2 - Make PContent::AsyncMessage and PContentBridge::AsyncMessage's data argument last.

There are also those SyncMessage messages from child to parent, and RpcMessage
Attachment #8739640 - Flags: review?(bugs) → review+
In case of InvokeDragSession, would it make sense to put visualData as last? it is quite large one. Though, there is also that array, which can be large too.
Attachment #8739642 - Flags: review?(jmathies) → review+
(In reply to Olli Pettay [:smaug] from comment #9)
> There are also those SyncMessage messages from child to parent, and RpcMessage

Those aren't quite as common, so I'm not going to bother with the code churn for now. The drag thing is something to think about. I see some very large messages for it. But if it isn't obvious which is largest, maybe it should just remain for now.
Keywords: leave-open
Whiteboard: [MemShrink]
Send/RpcMessage case is mostly about being consistent with the async variants, not necessarily about memory usage or anything like that.
Whiteboard: [MemShrink] → [MemShrink] btpp-active
Comment on attachment 8739641 [details] [diff] [review]
part 3 - Move PHttpChannel::OnTransportAndData's data argument last.

Maybe Honza could review this instead? It is a simple patch, and I'd like to get this uplifted to Aurora soon.
Attachment #8739641 - Flags: review?(honzab.moz)
Blocks: 1259183
Comment on attachment 8739641 [details] [diff] [review]
part 3 - Move PHttpChannel::OnTransportAndData's data argument last.

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

LGTM (you will have to merge after bug 1263028, but it will be simple)
Attachment #8739641 - Flags: review?(honzab.moz) → review+
tracking-e10s: --- → ?
Comment on attachment 8739641 [details] [diff] [review]
part 3 - Move PHttpChannel::OnTransportAndData's data argument last.

Thanks for the review! I landed both patches on inbound.
Attachment #8739641 - Flags: review?(jduell.mcbugs)
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Comment on attachment 8739638 [details] [diff] [review]
part 1 - Move PBrowser::AsyncMessage's data argument last.

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: increased memory usage with e10s, though unfortunately we can't exactly tell how much due to the telemetry we had
[Describe test coverage new/current, TreeHerder]: this code is run all over the place
[Risks and why]: low, it just changes the order IPC messages store their data in
[String/UUID change made/needed]: none
Attachment #8739638 - Flags: approval-mozilla-aurora?
Attachment #8739640 - Flags: approval-mozilla-aurora?
Comment on attachment 8739641 [details] [diff] [review]
part 3 - Move PHttpChannel::OnTransportAndData's data argument last.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8739641 - Flags: approval-mozilla-aurora?
Comment on attachment 8739642 [details] [diff] [review]
part 4 - Make PBrowserStream::Write's data argument last.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8739642 - Flags: approval-mozilla-aurora?
(answers are the same for all four parts)
I'd like to bake in central for 2-3 days before uplifting to Aurora.
Comment on attachment 8739638 [details] [diff] [review]
part 1 - Move PBrowser::AsyncMessage's data argument last.

Fix for e10s OOM crashes, this has baked in Nightly for ~a week now, Aurora47+
Attachment #8739638 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8739640 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8739641 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8739642 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-e10s: ? → ---
Should we add something to the ipdl processor that errors or warns when it sees someone do this? I'm concerned this problem will creep back into the code base over time.
Flags: needinfo?(continuation)
(In reply to Jim Mathies [:jimm] from comment #27)
> Should we add something to the ipdl processor that errors or warns when it
> sees someone do this? I'm concerned this problem will creep back into the
> code base over time.

Long-term, we should fix the IPC layer so that this does not waste a lot of space on internal fragmentation. Bug 1262671 should do this by bounding the fragmentation to whatever the size of an individual segment of data is, rather than approximately the entire size of the data.
Flags: needinfo?(continuation)
tracking-e10s: --- → +
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.