Andrew McCreight [:mccr8] Assignee
	Description • 8 years ago

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.)

	Andrew McCreight [:mccr8] Assignee
	Comment 1 • 8 years ago

"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_.

	Andrew McCreight [:mccr8] Assignee
	Comment 2 • 8 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 3 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e46a5015b436

	Andrew McCreight [:mccr8] Assignee
	Comment 4 • 8 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 7 • 8 years ago

This will reduce internal memory fragmentation for the IPC::Message used to make this call.

	Andrew McCreight [:mccr8] Assignee
	Comment 8 • 8 years ago

This will reduce internal memory fragmentation for the IPC::Message used to make this call.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 8 years ago

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.

	Pulsebot
	Comment 11 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/efd8dd10521a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2426b131185
https://hg.mozilla.org/integration/mozilla-inbound/rev/24e57e517a4d

	Andrew McCreight [:mccr8] Assignee
	Comment 12 • 8 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 8 years ago

Send/RpcMessage case is mostly about being consistent with the async variants, not necessarily about memory usage or anything like that.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/efd8dd10521a
https://hg.mozilla.org/mozilla-central/rev/f2426b131185
https://hg.mozilla.org/mozilla-central/rev/24e57e517a4d

	Andrew McCreight [:mccr8] Assignee
	Comment 15 • 8 years ago

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.

	Honza Bambas (:mayhemer)
	Comment 16 • 8 years ago

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)

	Pulsebot
	Comment 17 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d193db2fee91

	Andrew McCreight [:mccr8] Assignee
	Comment 18 • 8 years ago

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.

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d193db2fee91

	Andrew McCreight [:mccr8] Assignee
	Comment 20 • 8 years ago

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

	Andrew McCreight [:mccr8] Assignee
	Comment 21 • 8 years ago

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]:

	Andrew McCreight [:mccr8] Assignee
	Comment 22 • 8 years ago

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]:

	Andrew McCreight [:mccr8] Assignee
	Comment 23 • 8 years ago

(answers are the same for all four parts)

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 24 • 8 years ago

I'd like to bake in central for 2-3 days before uplifting to Aurora.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 25 • 8 years ago

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+

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d7ce5d17624
https://hg.mozilla.org/releases/mozilla-aurora/rev/846f5184333c
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc49e5e68ab3
https://hg.mozilla.org/releases/mozilla-aurora/rev/177a94ddc4b1

	Jim Mathies [:jimm]
	Comment 27 • 8 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 28 • 8 years ago

(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.