900268 - Trace actor collects too much information when serializing objects

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jake Bailey]

When serializing an object, the entire graph of reachable objects (via both ownProperties and the prototype) is serialized. This can result in huge amounts of data being sent over the remote debugging protocol on every function entry and exit. For example, `return {};` serializes Object.prototype and produces a 24,000 character packet: http://pastebin.mozilla.org/2696180

Some proposed solutions:

- Don't serialize prototypes. This shrinks packet sizes significantly for simple objects, but doesn't help with large data structure objects.

- Only serialize objects to a certain depth. This prevents large trees from being serialized, but doesn't solve the problem for objects that reference the global object, DOM nodes, or long arrays.

- Only serialize basic information about object parameters (like the class, possibly the list of names of ownProperties). This is similar to the previous suggestion, but with a max depth of 0. This solves the problem of individual large objects causing problematic packets, but makes the collected data not as useful.

The amount of information the user wants collected in the trace could potentially be an option--you'd trade performance for greater detail. Collecting everything (like we do now) is more detail than necessary for the majority of use cases, though.

Comment 1

[Jake Bailey]

Attachment: Patch

This patch serializes the ownProperties of argument and return value objects, but only creates an object grip for references in those objects (including the prototype reference). We could still get huge packets and slowdown when the arguments array is very long or when an object with a large ownProperties list is serialized...should we do something about that?

Example serialization output: http://pastebin.mozilla.org/2766675

Comment 2

[Jake Bailey]

Attachment: serialization.diff

Forgot to remove a deleted test from xpcshell.ini.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 785095 [details] [diff] [review]
Patch

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

Looks good to me, I think this is a practical trade off.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 785095 [details] [diff] [review]
Patch

Woops, old version of the patch

Comment 5

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 785105 [details] [diff] [review]
serialization.diff

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

tight

Comment 6

[Jake Bailey]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=1dbf0a1bc93c

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 785105 [details] [diff] [review]
serialization.diff

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

r+ with nit fix below

::: toolkit/devtools/server/actors/tracer.js
@@ +581,3 @@
>   */
> +function objectGrip(aObject) {
> +  let g = {

Nit: Can we use `grip` instead of `g`. I like to avoid one letter variable names unless they are in a single map/filter or whatever and only used once.

Comment 8

[Jake Bailey]

This function is one of the ones copied from script.js. These duplicated functions are eventually going to be in only one place, so I made as few changes as possible to make the refactoring easier.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/fx-team/rev/f98860de7350

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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