1391077 - Expanding a big array causes protocol slowness due to large packet size

Status: NEW

(DevTools :: Console, defect, P3)

Description

[Brian Grinstead [:bgrins]]

Blocking 1308566 for now but I'm not sure how much of this is also triggered by the variables view (the slowness is mostly on the backend for creating the Object Actors and sending the packet over).

STR:
Run this in the console: `Array(1e5).fill(0)`
Expand the Array

Profile:
https://perf-html.io/public/08792e9ab97ffa7f180ae5376cc38bf73be4375b/calltree/?hiddenThreads=&implementation=js&range=2.1041_3.2990&thread=2&threadOrder=0-2-3-4-1

About half of the time is spent in the onPrototypeAndProperties handler (ObjectActor) and the other half is spent sending the huge packet back to the parent process.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: packet.txt

Here's the packet this ends up sending (gathered with devtools.dump.emit and browser.dom.window.dump.enabled prefs on).

A couple ideas (not mutually exclusive):
1) Turn the configurable/writable/enumerable properties in the packet into a bitmask or something much smaller than spelling each out (backwards incompatible RDP change)
2) Load only the buckets on the initial expansion and then wait to fetch properties until the buckets are expanded

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

The frontend part is already tracked in https://github.com/devtools-html/devtools-core/issues/552 .
I think this is the easiest way to go for now: the variable view was doing it that way so we have the server functions to load only slices of an object properties.

Bug 1387823 is also related I think

Comment 3

[Alexandre Poirot [:ochameau]]

See bug 1023386 to see how it has been implemented in the old console.
I used to replicate chrome's behavior at the time I implemented that bug.
The slicing algorithm is quite simple. We split the property names into 4 buckets, everytime we have more than 2000 of them. So that we are sure that we never display too many slices, we only display 4 slices.

Otherwise I don't know if you already use client.enumProperties and its `iterator` feature?
  http://searchfox.org/mozilla-central/source/devtools/shared/client/main.js#2607-2635
  http://searchfox.org/mozilla-central/source/devtools/shared/client/main.js#2767-2837

It looks like you do:
  http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/actions/messages.js#81
But without really using its main feature. You shouldn't fetch slices from 0 to iterator.count, but instead only slices what you have to display. iterator actor has a "names" request which helps knowing what is the name of a property at a given offset. I don't know if you added slicing for non-array object, this is also important to support slicing on objects with many properties, like huge JSON objects.

Comment 4

[Brian Grinstead [:bgrins]]

We could also consider compressing big packets before sending cross process - this particular packet is 7.4MB and after gzipping it is 270KB

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> We could also consider compressing big packets before sending cross process
> - this particular packet is 7.4MB and after gzipping it is 270KB

This is done for Telemetry: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySend.jsm#147

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > We could also consider compressing big packets before sending cross process
> > - this particular packet is 7.4MB and after gzipping it is 270KB
> 
> This is done for Telemetry:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/
> TelemetrySend.jsm#147

Alex, have we thought about doing this before? And do you have any ideas how we could handle backwards compatibility for older clients that wouldn't support receiving compressed packets?

Comment 7

[Alexandre Poirot [:ochameau]]

It may be worth looking at for cases where we really have to pass big chunk of data, like http bodies in network monitor.
But here, it looks like we should lazily load things.
FYI, the sole fact of creating the grip for all property values will slow things down a lot, this isn't only about RDP/message manager being slow.

Also instead of compressing, I think we should look into:
  http://searchfox.org/mozilla-central/source/dom/webidl/StructuredCloneHolder.webidl
And see if we can make specific optimization on non-remote communications (i.e. on Firefox Desktop).

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> It may be worth looking at for cases where we really have to pass big chunk
> of data, like http bodies in network monitor.
> But here, it looks like we should lazily load things.
> FYI, the sole fact of creating the grip for all property values will slow
> things down a lot, this isn't only about RDP/message manager being slow.

I don't see it as an either / or for this particular bug. We should definitely lazily load the buckets, but even individual buckets can get quite large depending on the size and contents of the array, and other scenarios would benefit from improved transfer for big packets.

> Also instead of compressing, I think we should look into:
>  
> http://searchfox.org/mozilla-central/source/dom/webidl/StructuredCloneHolder.
> webidl
> And see if we can make specific optimization on non-remote communications
> (i.e. on Firefox Desktop).

I'd be happy to focus optimizations on non-remote transport. It will get rid of backwards compatibility issues as well.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> Also instead of compressing, I think we should look into:
>  
> http://searchfox.org/mozilla-central/source/dom/webidl/StructuredCloneHolder.
> webidl
> And see if we can make specific optimization on non-remote communications
> (i.e. on Firefox Desktop).

Looked a bit more into this and AFAICT the messageManager is already using this: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameMessageManager.cpp#625

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Bug 1399460 made expanding the array from Comment 0 fast. SHould we re-purpose this bug to investigate with the protocol side of things ?

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)
> Bug 1399460 made expanding the array from Comment 0 fast. SHould we
> re-purpose this bug to investigate with the protocol side of things ?

Yes, good idea. We should also investigate how chrome RDP handles this.

Comment 12

[Brian Grinstead [:bgrins]]

Talked with billm about this, and most likely zipping won't help since the cost to serialize the object in the first place will be similar to the structured clone. Best bet is to somehow shorten the packet (either simplify the descriptor format, have a way to specify repeated values, etc). Also, let's check into the chrome protocol handles object previews and see if it's smaller to start with.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Could we have some sort of a keys dictionary sent in the packet to reduce the size ? Like : 
```
{
  mapping: {
    0: "from",
    1: "to",
    2: "ownProperties",
    3: "prop-a",
    4: "prop-b",
  },
  0: "actor1",
  1: "actor2",
  2: {
    3: "x",
    4: []
  }
}
```
And we could then recreate the original object.
Not sure 1. we want this for every packet, 2. it will enhance performance, since we have to go through the tree to build the dictionary.