1188526 - Pageload much slower when Inspector is open in a specific case

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Loading the attached testcase (originally from <http://rcocks.github.io/>) is much slower when Inspector is open than when it's not open.

I profiled this a bit, and of the 7s of wall-clock time in the profile about 2.5s are Proxy::get which does various proxy and Xray goop and then lands in xpc::AddonWrapper<xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits> >::get which very eventually calls a DOM binding getter.  Of those 2.5s, only about 270ms are under the binding getter.

The binding getters involved are the "type", "target", "attributeName", "attributeNamespace" getters on MutationRecord objects.

Another 1.7s wall-clock is under Proxy::callProp, whih mostly spends its time (1.4s) under Proxy::get doing Xray stuff.  In fact, I can't even tell what we're calling because all the samples were under Proxy::get afaict.

Also, about 600ms doing JS_WrapValue whie calling MutationCallback::Call (presumably this is the chrome MutationCallback we're calling here, so have to Xray-wrap all the stuff).  The vast majority of this is actual js::NewObjectWithGivenTaggedProto.

Apart from that, there's a lot of .value sets on HTMLInputElements (about 1s wall-clock under here).  That's done by the actual web page code.

So it looks to me like having lots of value attribute changes like this queues a ton of mutation observers and some combination of our Xray code and our devtools ends up not that happy with that...

Comment 1

[Bobby Holley (:bholley)]

We could almost certainly optimize the Xray code more. I'd rather wait to do tht until we kill off XPCWrappedNative Xrays (which we could maybe do already?).

Is there any way we could be smarter about this on the devtools side?

Comment 2

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

We could also add something ([ChromeOnly] for now) to MutationObserver to skip similar records. 
Perhaps something like
partial dictionary MutationObserverInit
{
  boolean mergeAttributeRecords; // default to false
};

where adjacent similar records would be merged so that intermediate values could be skipped.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We can't kill XPCWrappedNative Xrays until we finish moving web-facing stuff to Web IDL bindings.  :(

Comment 2 sounds like it might work pretty well for the inspector use case.

Comment 4

[Julian Descottes [:jdescottes]]

The situation here improved significantly when moving from Firefox 41 to Firefox 42. The page no longer hangs on load. Since Bug 1188640 was delivered in Firefox 42, that might explain the difference.

Even though the logged timings are a bit higher with the inspector opened, I don't know if this is something that needs to be addressed? 

On my machine timings go from 170ms w/o inspector to 230ms w inspector.

jryans : can you test again and assess if we should keep this bug open ?

Comment 5

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

(In reply to Julian Descottes [:jdescottes] from comment #4)
> The situation here improved significantly when moving from Firefox 41 to
> Firefox 42. The page no longer hangs on load. Since Bug 1188640 was
> delivered in Firefox 42, that might explain the difference.
> 
> Even though the logged timings are a bit higher with the inspector opened, I
> don't know if this is something that needs to be addressed? 
> 
> On my machine timings go from 170ms w/o inspector to 230ms w inspector.
> 
> jryans : can you test again and assess if we should keep this bug open ?

I agree it does seem better now.  I know that :bgrins and :pbro had some additional improvement ideas, such as bug 1171482.  Let's ask :bgrins if this bug should stay open pending more experiments.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> (In reply to Julian Descottes [:jdescottes] from comment #4)
> > The situation here improved significantly when moving from Firefox 41 to
> > Firefox 42. The page no longer hangs on load. Since Bug 1188640 was
> > delivered in Firefox 42, that might explain the difference.
> > 
> > Even though the logged timings are a bit higher with the inspector opened, I
> > don't know if this is something that needs to be addressed? 
> > 
> > On my machine timings go from 170ms w/o inspector to 230ms w inspector.
> > 
> > jryans : can you test again and assess if we should keep this bug open ?
> 
> I agree it does seem better now.  I know that :bgrins and :pbro had some
> additional improvement ideas, such as bug 1171482.  Let's ask :bgrins if
> this bug should stay open pending more experiments.

I'm fine with closing this one out, since it seems like really the bulk of this was resolved with Bug 1188640.  What would be great is if we could add a new DAMP measurement using a similar test case so we could get better tracking for when things like Bug 1171482 get worked on.  I'll file a different bug for that.