1263004 - Many large debug:netmonitor:*.*.*/*:updateEvent messages in the parent process

Status: RESOLVED WONTFIX

(DevTools :: Netmonitor, defect)

Description

[Andrew McCreight [:mccr8]]

Attachment: net monitor message size telemetry

There are many large messages of the form debug:netmonitor:*.*.*/*:updateEvent being sent from the parent process, according to MESSAGE_MANAGER_MESSAGE_SIZE telemetry, thousands of them above 1MB. Large messages require contiguous address space, which can result in high rates of OOM crashes with e10s.

Comment 1

[Andrew McCreight [:mccr8]]

These are not super common compared to the other large messages, and people using the debugger probably have better machines than the average user, so this can probably wait, but I figured I would file the bug.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

These messages are shipping complete network request data around, so on the whole I am not surprised there are many > 1MB.

As you say, it only impacts people using the network monitor tool.  What would be the "right" way to move this data around?  Break it up into smaller pieces?  Not use the IPC mechanism at all, but some other facility instead?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Bill and Andrew, do you have a good suggestion about how to handle transferring large data through the message manager? I think we'll need a similar guidance over in bug 1262661

Comment 4

[Andrew McCreight [:mccr8]]

I'm not really sure, sorry.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Hmm, message manager just uses whatever API IPC layer provides easily.
mccr8, do you think it would help if IPC layer had some way to split large messages to smaller pieces 
(say 4k), and then it would be up to the receiver to deal with non-contiguous data?
In message manager case it would probably require us to change structured cloning handling in js engine.

But for short time fix, it is up to the user of message manager to not sent massive messages, those need to be split to smaller pieces in JS.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm having a hard time figuring out where the data is actually being sent here. Is there just a big string somewhere? The code is really generic and hard to follow.

If it is a big string, then it should be broken up into smaller strings. Hopefully each one could be processed incrementally on the receiving side without needing to assemble them all together again. If we do need them all, it would be better to store an array of strings rather than one big string, if possible.

Regarding improving the API, it looks like it wouldn't be too hard to add a new API for structured cloning that returned the data non-contiguously. That wouldn't help if the data is a giant string, but it would help for big structured data (like session store).

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Bill McCloskey (:billm) from comment #6)
> Regarding improving the API, it looks like it wouldn't be too hard to add a
> new API for structured cloning that returned the data non-contiguously. That
> wouldn't help if the data is a giant string, but it would help for big
> structured data (like session store).
Well, it would help even in string case. We'd store the data in SC mode using several smaller blocks and only when deserializing, we'd need to allocate large block using whatever allocation method JS engine uses. Right now, as far as I see, IPC layer allocates a large block, then SC reads and allocates a separate large block for the result, and then IPC layer deallocates.


Perhaps SCOutput/SCInput could use SegmentedVector or some such. Though, need to figure out how to easily pass the data to/from IPC layer. sphink might have some ideas here?

Anyhow, for this particular bug sounds like devtools just need to not pass massive messages.

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #5)
> mccr8, do you think it would help if IPC layer had some way to split large
> messages to smaller pieces (say 4k), and then it would be up to the receiver to deal with
> non-contiguous data?

Yeah, that would be good. I'm not sure exactly how that would work or how hard it would be.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Bill McCloskey (:billm) from comment #6)
> I'm having a hard time figuring out where the data is actually being sent
> here. Is there just a big string somewhere? The code is really generic and
> hard to follow.

The largest message I would expect for the network monitor is when it collects the response body[1] for a request.  The goes from the main process to the child[2] because requests happen in the main process, but the child owns the rest of the DevTools data about the tab.  However, the DevTools UI is actually communicating with the main process, so this data will eventually get shipped across the DevTools connection, and for a child process tab that connection is... the message manager!  So, in fact this data goes round trip back to the process it started from.  The design of all this could certainly use some reworking clearly, it sort of evolved organically in some ways, but that's a complex change to make for something only affecting DevTools usage.

We currently limit response body tracking to 1 MB per request (we only hold onto the first 1 MB)[3], so the message should not go **too far** above 1 MB, since there's some surrounding metadata as well as the response body itself.

However, looking at the telemetry, it appears you are seeing some in the multiple MB range, which surprises me somewhat.  Unfortunately it appears the message manager message type that gets used is generic for all the bits of the request we send here, so this telemetry probe probably can't tell us which specific types are so large.

So, I could imagine several DevTools specific things to look into:

* Which messages are going far above 1 MB and why?  (Maybe more specific probes would help.)
* Can we chop up the messages into smaller chunks?
* Maybe we should finally redesign the network monitor to stop shipping data all over the place

[1]: https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/devtools/shared/webconsole/network-monitor.js#399-402
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#1375
[3]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#37

Comment 10

[Andrew McCreight [:mccr8]]

I filed bug 1264642 about making improvements at the structured clone level. I think we should leave this to improvements that could be made at the devtools level.

Comment 11

[Jim Mathies [:jimm]]

Can we get this assigned to someone and looked at? OOM issues are currently holding up e10s rollout.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jim Mathies [:jimm] from comment #11)
> Can we get this assigned to someone and looked at? OOM issues are currently
> holding up e10s rollout.

:jwalker, how should we handle this?  Since this one is specific to people who are using the DevTools, it may not make sense to block e10s on this one.

Comment 13

[Andrew McCreight [:mccr8]]

If there's no easy fix for this, we can WONTFIX it. This isn't very common, and bug 1262671 will deal with the contiguous address space problem.

Comment 14

[Joe Walker [:jwalker]]

Just been talking to Jeff, we're thinking that this probably shouldn't block e10s. Many people are using devtools with e10s on already, and we didn't know about this problem.

It certainly feels wrong to prioritize a bug that we only know about in theory over bugs that people regularly complain about.

Comment 15

[Andrew McCreight [:mccr8]]

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #14)
> It certainly feels wrong to prioritize a bug that we only know about in
> theory over bugs that people regularly complain about.

This isn't just theoretical, but it isn't a super common issue, so I'll just WONTFIX it for now.