1449231 - Investigate significant regression on DAMP tests after Bug 888600

Status: RESOLVED DUPLICATE of bug 1474843

(DevTools :: General, defect, P2)

Description

[Julian Descottes [:jdescottes]]

We spotted a significant regression on some of the DevTools talos tests after Bug 888600 landed:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=18a940f854bfa1df873b9b1701ecb6b9e5e3ce59&newProject=try&newRevision=0376052261bfaeba6b827acb12d74eecbf9397f9&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1

Especially custom.inspector.open and  custom.inspector.reload regressed by 30% and 40%.

We still have to check if this is an actual regression for end users, or only visible in tests.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: test_case.html

This is a heavier version of our DAMP test file. 

Using the STRs:
- open inspector
- select the first child of the body in the markup view
- Reload

Recorder two profiles
- before Bug 888600: https://perfht.ml/2J0ukq8
- after Bug 888600: https://perfht.ml/2pKDSfS
=> I cannot spot a clear difference between the two profiles.

I also ran locally DAMP before and after and I have not seen a significant difference. I am testing on OSX, but perfherder reports the same regression on OSX as on other platforms.

Comment 2

[Julian Descottes [:jdescottes]]

Sorry I must have used wrong changesets while testing previously. Here I used
- https://hg.mozilla.org/mozilla-central/rev/8af210a4d4d0 (changeset right before Bug 888600)
- https://hg.mozilla.org/mozilla-central/rev/243246b2f440 (last changeset for Bug 888600)

Which gives the following results:
- before: https://perfht.ml/2I8fHj2
- after : https://perfht.ml/2I4moTl

This time the profile for the "after" looks much different. Purely in JS we can see that node-attribute-parser.js getAttribute and hasAttribute [1] take a huge time. Combined stacks highlight js::NurseryAwareHashMap::lookup.

The difference in performance is also clearly noticeable manually.

[1] https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/devtools/client/shared/node-attribute-parser.js#240-256

Comment 3

[Julian Descottes [:jdescottes]]

The following change "fixes" most of the performance issue:

# HG changeset patch
# User Julian Descottes <[e-mail]>
# Date 1522172167 -7200
#      Tue Mar 27 19:36:07 2018 +0200
# Node ID f7b2dae857118832d2a1c264058b0d5f714bb1d1
# Parent  243246b2f4406f0b2f118758220db5c7fdd215df
workaround for perf regression

MozReview-Commit-ID: 2sE2tIpH5YE

diff --git a/devtools/client/inspector/markup/views/element-editor.js b/devtools/client/inspector/markup/views/element-editor.js
--- a/devtools/client/inspector/markup/views/element-editor.js
+++ b/devtools/client/inspector/markup/views/element-editor.js
@@ -444,17 +444,18 @@ ElementEditor.prototype = {
 
     this.removeAttribute(attribute.name);
     this.attrElements.set(attribute.name, attr);
 
     // Parse the attribute value to detect whether there are linkable parts in
     // it (make sure to pass a complete list of existing attributes to the
     // parseAttribute function, by concatenating attribute, because this could
     // be a newly added attribute not yet on this.node).
-    let attributes = this.node.attributes.filter(existingAttribute => {
+    let attributes = [...this.node.attributes];
+    attributes = attributes.filter(existingAttribute => {
       return existingAttribute.name !== attribute.name;
     });
     attributes.push(attribute);
     let parsedLinksData = parseAttribute(this.node.namespaceURI,
       this.node.tagName, attributes, attribute.name);
 
     // Create links in the attribute value, and collapse long attributes if
     // needed.

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #2)
> Which gives the following results:
> - before: https://perfht.ml/2I8fHj2
> - after : https://perfht.ml/2I4moTl

Looking at these profiles, it looks like the is a lot of additional work around wrappers (js::CrossCompartmentWrapper::call).
As if we weren't involving wrappers before and now we do.

This is surprising. Bug 888600 seems to be so far from this code.
Bug 888600 seems to involve changes around wrappers as I saw various additional call to wrapObject helpers in its patches.
But the message manager is used at RDP level, in devtools/server/{main,child}.js. It is quite far from this code in element-editor.js!

element-editor.js should involve cross compartment wrappers.
element-editor.js module is loaded via base-loader.js into a privileged scope, via a Sandbox. All inspector modules are loaded into the same global/compartment (like jsm now). But `node` and `node.attributes` are from another compartment, it comes from inspector.xhtml document.
So there should always be cross compartment wrappers here.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #2)
> > Which gives the following results:
> > - before: https://perfht.ml/2I8fHj2
> > - after : https://perfht.ml/2I4moTl
> 
> Looking at these profiles, it looks like the is a lot of additional work
> around wrappers (js::CrossCompartmentWrapper::call).
> As if we weren't involving wrappers before and now we do.
> 
> This is surprising. Bug 888600 seems to be so far from this code.
> Bug 888600 seems to involve changes around wrappers as I saw various
> additional call to wrapObject helpers in its patches.
> But the message manager is used at RDP level, in
> devtools/server/{main,child}.js. It is quite far from this code in
> element-editor.js!
> 
> element-editor.js should involve cross compartment wrappers.
> element-editor.js module is loaded via base-loader.js into a privileged
> scope, via a Sandbox. All inspector modules are loaded into the same
> global/compartment (like jsm now). But `node` and `node.attributes` are from
> another compartment, it comes from inspector.xhtml document.
> So there should always be cross compartment wrappers here.

We just discussed this, and the `node` used in element-editor.js is a NodeFront instance and not a real node from inspector.xhtml. This means that node.attributes is an array which has been copied over via RDP. The array is created at https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/devtools/shared/fronts/node.js#160 

It seems that a change from Bug 888600 makes this array use a different compartment than before. 
Peter, does this make sense to you, do you think this is an expected change?

Comment 6

[Alexandre Poirot [:ochameau]]

Here is some pointer to help see where we are involving a message manager.
`_form.attrs` ultimately comes from a message manager message.
It should come from this receiveMessage call:
  https://searchfox.org/mozilla-central/source/devtools/server/main.js#1070
Related to the message manager we use here:
  https://searchfox.org/mozilla-central/source/devtools/server/main.js#1001-1004

`mm` is the message manager for the <xul:browser> element that devtools are debugging.

Could it be that the objects within messages have a different compartment your patch?

Comment 7

[Peter Van der Beken [:peterv]]

Could be. I'll take a look.

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> element-editor.js module is loaded via base-loader.js into a privileged
> scope, via a Sandbox. All inspector modules are loaded into the same
> global/compartment (like jsm now).

So the NodeFront instance is created in the sandbox? And how is the connectToFrame code related to the sandbox?
I suspect the array comes from the same compartment as the onActorCreated listener, could you check that? And that is not the compartment of the sandbox?

Comment 9

[Alexandre Poirot [:ochameau]]

(In reply to Peter Van der Beken [:peterv] from comment #8)
> (In reply to Alexandre Poirot [:ochameau] from comment #4)
> > element-editor.js module is loaded via base-loader.js into a privileged
> > scope, via a Sandbox. All inspector modules are loaded into the same
> > global/compartment (like jsm now).
> 
> So the NodeFront instance is created in the sandbox?

Yes.

> And how is the connectToFrame code related to the sandbox?

connectToFrame's onActorCreated doesn't relate to NodeFront.
onActorCreated will be first only once, for a special "tab actor", which will later allow to fetch the "node actor" used by NodeFront. This node actor will be fetched by the transport layer.
The receiveMessage function that will eventually create the NodeFront and the problematic `node.attributes` array is this one:
  ChildDebuggerTransport.receiveMessage:
  https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/devtools/shared/transport/transport.js#758
You get the connection to connectToFrame here:
  https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/devtools/server/main.js#1064-1070
        // Pipe Debugger message from/to parent/child via the message manager
        childTransport = new ChildDebuggerTransport(mm, prefix);
        childTransport.hooks = {
          onPacket: connection.send.bind(connection),
          onClosed() {}
        };
        childTransport.ready();

> I suspect the array comes from the same compartment as the onActorCreated
> listener, could you check that? And that is not the compartment of the
> sandbox?

Hum. Is there a way to check that in JS?
Does you patch change anything regarding the compartment used for the `data` object passed to `receiveMessage`?

Comment 10

[Alexandre Poirot [:ochameau]]

The original issue being this regression is being fixed in bug 1474843.