I'm not sure it is specific to devices. We may not notice the regression on desktop thanks to bigger CPU... I'm not sure it is inspector specific as well, but we can easily feel the regression when browsing the DOM! It takes a significant time opening children in the tree view.

Is it a recent regression?

Could it be related to the new inspector?

Given the feedback I received from gaia devs, it looks quite recent no longer than a month. More like one/two week(s).

I tested a little bit on my Peak. It does feel slow, probably slower than before, but I'm haven't got enough past experience debugging on remote devices to tell the different for sure. What I can say though is that it mostly is about the style inspector tab. The highlighter appears to be quite fast, and expanding new nodes too (once you've selected them before and waited for the styles to load). What does feel really slow is the time it takes between the time you select a node and the time the styles appear on the right-side.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4) > What does feel really slow is the time it takes between the time you select > a node and the time the styles appear on the right-side. And btw, this time is mostly spent waiting for the response packet.

I tried to revert the changeset that fixed webcomponents and it is still slow :'( I now got report of gaia developers not being able to work fullspeed because of this. We should really backout the regression asap.

I tried to backout some inspector patches without any visible win. I also tried to disable the highlighter by disabling all `show` methods in actors/highlighter.js without any noticeable win either. I'm starting to wonder if that is a general protocol/server slowness or if it is only slow in the inspector. For example, the following code in the console looks very slow: window.setInterval(()=>console.log("foo"), 0);

Should we have QA hunt for a regression window? Is the slowness only on real devices, or does it happen in the simulator too?

The inspector slowness seems pretty dependent on the number of stylesheets and cssrules in my test. Deleting all rules using the style-editor makes the inspector feel a lot more snappy.

I can see an important different in the time it takes to display the rule-view between the simulator 1.3 and the simulator 2.2. The main difference I see between the 2 is the number of css variables used in 2.2. There are many more rules and properties to be displayed in the rule-view with the 2.2 version and so it takes more time. We had already seen performance problems with this panel (see bug 1020894), maybe that's just it.

It is especially slow on the getComputed request. It take ~2s for any node on my unagi :s Isn't there something wrong, even if this returns a big list, shouldn't this be faster? Bug 1103993 is really great, I like seeing more and more lazyness in devtools! But it will still be slow to open the "Computed" panel! FYI, all other requests have reasonable response type < 250ms.

(In reply to Alexandre Poirot [:ochameau] from comment #11) > It is especially slow on the getComputed request. It take ~2s for any node > on my unagi :s > Isn't there something wrong, even if this returns a big list, shouldn't this > be faster? Yes it should! I'll take a look at why getComputed is slow after I'm done with bug 1103993, unless someone beats me to it. There may be stuff we do in css-logic that make this slow. Getting the list of computed styles should be really fast, maybe the slow part is where we try and conciliate this list with css rules (this is used when you expand a computed property, so we can give you a trace of where this property came from). If this is the reason, we could do this lazily.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12) > (In reply to Alexandre Poirot [:ochameau] from comment #11) > > It is especially slow on the getComputed request. It take ~2s for any node > > on my unagi :s > > Isn't there something wrong, even if this returns a big list, shouldn't this > > be faster? > Yes it should! I'll take a look at why getComputed is slow after I'm done > with bug 1103993, unless someone beats me to it. > There may be stuff we do in css-logic that make this slow. > Getting the list of computed styles should be really fast, maybe the slow > part is where we try and conciliate this list with css rules (this is used > when you expand a computed property, so we can give you a trace of where > this property came from). If this is the reason, we could do this lazily. Except that this is already lazily retrieved ... my bad.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12) > Yes it should! I'll take a look at why getComputed is slow after I'm done > with bug 1103993, unless someone beats me to it. > There may be stuff we do in css-logic that make this slow. > Getting the list of computed styles should be really fast, maybe the slow > part is where we try and conciliate this list with css rules (this is used > when you expand a computed property, so we can give you a trace of where > this property came from). If this is the reason, we could do this lazily. It looks like what is slow is this: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/styles.js#166 166 Array.prototype.forEach.call(computed, name => { 167 let matched = undefined; 168 ret[name] = { 169 value: computed.getPropertyValue(name), 170 priority: computed.getPropertyPriority(name) || undefined 171 }; 172 }); It takes >80% of the time spent in getComputed, something around 800ms on my unagi. This code hasn't changed for quite a while, so either we have way more items in `computed` or getPropertyValue/getPropertyPriority regressed, or "there is some dark matter around".

As discussed over IRC, this a fix that makes PageStyleActor.getComputed() take ~250ms on my peak, as compared to ~700ms (tested on the Settings app). I don't know why it's the case, but rule.getPropertyValue(property) seems to be slower than it used to be. Or perhaps it's just that apps on B2G have more rules than before. Each call to getPropertyValue takes about ~1ms on my peak, but we call it for (each rule X each computed property), so this ends up taking a long time. Let me know what you think.

Comment on attachment 8529041 [details] [diff] [review] bug1098435-make-getcomputed-faster v1.patch Review of attachment 8529041 [details] [diff] [review]: ----------------------------------------------------------------- I'm not used to work with/review this code, but given my comprehension of this particular method, it looks good. ::: toolkit/devtools/styleinspector/css-logic.js @@ +596,5 @@ > + if (cssText.indexOf(property+":") == -1) { > + return true; > + } > + > + let isInherited = domUtils.isInheritedProperty(property); Note that by setting isInheritedProperty result in a variable, you do not let a chance to *not execute* this method if isMatched is true or isParentMatched is false. Unfortunately, JS isn't Haskell and isn't lazy by default ;)

Thanks for the review. Good catch on isInheritedProperty, I'll correct this in a bit. I found another possible cause for the overall slowness of the inspector: add a log to the getLayout method and then move your mouse over the nodes in the markup-view: every time a node is hovered, the method is called twice. It takes about 500ms for the first call and then about 15ms for the second call. It looks to me like we can do away with these calls. I will take a look at this next.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #17) > I found another possible cause for the overall slowness of the inspector: > add a log to the getLayout method and then move your mouse over the nodes in > the markup-view: every time a node is hovered, the method is called twice. > It takes about 500ms for the first call and then about 15ms for the second > call. > It looks to me like we can do away with these calls. I will take a look at > this next. I think I found out why this is the case. Btw, this only happens when the layout-view panel is visible. The highlighter seems to be causing sync reflows in the page. STR: - open the inspector on any page - open the split console - turn on CSS reflow logging - hover over nodes in the markup-view => synced reflows are logged: reflow: 0.08ms function LayoutHelpers.prototype.getAdjustedQuads, LayoutHelpers.jsm line 44 reflow: 0.22ms function BoxModelHighlighter.prototype<._updateInfobar, highlighter.js line 1252 These lines are where we do node.getBoundingClientRect and node.getBoxQuads. There is no change here, we've been doing this for a while, BUT, what has changed is that the highlighter is now part of the page (see bug 985597). And even if it's a native anonymous node it somehow seems to make sync reflow needed when accessing APIs like getBoxQuads.

At least, this reduces the number of synced reflows from 2 to 1, but I still need to figure out why the highlighter causes reflows at all.

Oh and to be clear, this is bad because the layout-view listens to reflows, and uses them to refresh (and to do so, it calls both getLayout and getApplied). So this for sure contributes to the slowness.

Comment on attachment 8529041 [details] [diff] [review] bug1098435-make-getcomputed-faster v1.patch Review of attachment 8529041 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/styleinspector/css-logic.js @@ +592,5 @@ > + // If the property isn't found in the rule's cssText, early return and > + // keep the property in the list for the next rule. > + // Note that it's faster to search the string than to use > + // rule.getPropertyValue(property) > + if (cssText.indexOf(property+":") == -1) { Hmm. searching text may be faster, but it's always more error-prone than parsing. Here in particular, property "top" will yield a non -1 index if "border-top:" exists in the cssText.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #21) > Hmm. searching text may be faster, but it's always more error-prone than > parsing. Here in particular, property "top" will yield a non -1 index if > "border-top:" exists in the cssText. cssText seems to always format declarations with a space in between, so searching for " " + property + ":" would be a lot safer. In fact it would only fail if this particular string happened to be in a font-family name, or url, or content value.

Paul, we've had a quite lengthy discussion yesterday on IRC about how to make the highlighter not cause reflows, but it turns out it wasn't too hard to actually just ignore these reflows. Here's a patch that does just that.

Comment on attachment 8529742 [details] [diff] [review] bug1098435-highlighter-causes-reflows v2.patch In setIgnoreLayoutChanges, you might also want to flush reflows when ignore == true and before setting gIgnoreLayoutChanges to true, to give a change to any user of the actor to catch pending reflows.

Thanks Paul for the review. I updated the patch as per your comments. Here's a pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e9b11020c75c

This patch makes searching for properties in the rules' cssText safer, but it's not perfect. Brian, as described in comment 21 and comment 22, this isn't an ideal solution because the text we search for may appear in a value. I'm interested if you have any other ideas on how to make the patched function faster. Using rule.getPropertyValue is slow due to the number of times we call it (many rules + many properties), and parsing the cssText ourselves might be pretty slow too.

Comment on attachment 8530175 [details] [diff] [review] bug1098435-highlighter-causes-reflows v3.patch Pushed this patch to fx-team: https://hg.mozilla.org/integration/fx-team/rev/388f0495a050

CCing Mike as he said he was refactoring the css-logic module.

Comment on attachment 8530176 [details] [diff] [review] bug1098435-make-getcomputed-faster v2.patch Mike, is this change going to be unnecessary after the refactors you are working on? And what is the bug # where you are doing this work?

(In reply to Brian Grinstead [:bgrins] from comment #30) > Comment on attachment 8530176 [details] [diff] [review] > bug1098435-make-getcomputed-faster v2.patch > > Mike, is this change going to be unnecessary after the refactors you are > working on? And what is the bug # where you are doing this work? As a personal project I am refactoring the computed view to use React. The reason I am doing this is to gain insights as to the advantages and issues we will face if we decided to move our codebase over to react. In my opinion this is the only way we can gain a realistic appreciation of the issues that such a migration would cause. So far I can see the following advantages: - Much simpler code to manage the store representing the user interface. In many instances the code to e.g. calculate styles, urls etc. is greatly simplified as we don't need to worry about granular changes to the user interface. - Simpler to add new features. - Less bugs. - Easier to find and fix bugs, especially for new contributors. - Easier to create tests, also testing each individual component and sub-component is a *huge* win. - Far less DOM changes so a much faster app. - Because each tab would be a component we could e.g. render all of our tools in one test page to greatly simplify theming. - If we use Flux, and we totally should, then the code scales without becoming overly complex (http://tinyurl.com/flux-react-png ). This is completely different to using MVC libraries which become more complex as the app progresses (http://tinyurl.com/flux-react-mvc-png ). Anyhow, the store I have created is designed to hold the complete app state so it is of no use without also converting the computed view to use React or another similar library. I will leave myself needinfod on this bug so that I can come back and give more feedback when I have made more progress.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #31) > So far I can see the following advantages: > - Much simpler code to manage the store representing the user interface. In > many instances the code to e.g. calculate styles, urls etc. is greatly > simplified as we don't need to worry about granular changes to the user > interface. > - Simpler to add new features. > - Less bugs. > - Easier to find and fix bugs, especially for new contributors. > - Easier to create tests, also testing each individual component and > sub-component is a *huge* win. > - Far less DOM changes so a much faster app. > - Because each tab would be a component we could e.g. render all of our > tools in one test page to greatly simplify theming. > - If we use Flux, and we totally should, then the code scales without > becoming overly complex (http://tinyurl.com/flux-react-png ). This is > completely different to using MVC libraries which become more complex as the > app progresses (http://tinyurl.com/flux-react-mvc-png ). All of this is really cool, but so far, the main perf bottleneck we've noticed with the inspector happens on the server-side, in css-logic, especially in cssLogic.hasMatchedSelectors, which is called when PageStyleActor.getComputed(). You mentioned last week that the experiment you describe here also contained a css-logic refactor that should help with performance. Anything you can comment about now?

Comment on attachment 8530176 [details] [diff] [review] bug1098435-make-getcomputed-faster v2.patch Clearing feedback while awaiting info about the css-logic refactor from Mike in comment 32. Sounds like that would be a great change for us here, and hopefully decoupled from the react changes.

(In reply to Brian Grinstead [:bgrins] from comment #33) > Comment on attachment 8530176 [details] [diff] [review] > bug1098435-make-getcomputed-faster v2.patch > > Clearing feedback while awaiting info about the css-logic refactor from Mike > in comment 32. Sounds like that would be a great change for us here, and > hopefully decoupled from the react changes. It would need to be part of a react based implementation as you use stores differently than normal objects. A store represents the whole component so replacing sections of objects or even the whole object is fine because of the DOM diffing. This would be very inefficient with our current implementation. I dropped that a while back as it didn't look like any managers want to switch to an external library. I don't even have the code any more.

Patrick, do we still need to leave this bug open? Also, I'm not sure b2g is still a priority for us now.

Let's close it. Some changes have landed 2 years ago. Let's open a new bug if/when we have perf problems again in the future.