The much larger number of non-numerable properties available on window take a pretty long time to fetch via the protocol, and an extra fair bit of time to build the tree. We can try a few things, at least to give the illusion of faster performance: * only for things like 'this' and 'window', fetch the prototype and properties on hover, so that after clicking they're immediately available and don't block expansion * don't provide an expansion animation for nodes with more than a certain number of properties (50? 100? to be determined) * play with the thread manager and dispatch draw operations to paint nodes as soon as possible in the tree

There's only so much that can be done to give the illusion of better performance. All the ideas mentioned in the first comment helped a lot with the perceived responsiveness when expanding nodes, but there's still a certain degree of bottleneck felt only when rapidly stepping through code (because there's all this huge pile data about the global scope that has to be carried before resuming is allowed). A few additions: * 'window' and 'this' are no longer allowed to be re-expanded after pauses * removed a few css rules that were heavy on rendering the tree I think this patch is enough and there's no need for protocol changes yet. Debug builds are still rather slow, but optimized ones are fast enough imho. Let's hope this patch survives try :)

Ermahgerd it worked... https://tbpl.mozilla.org/?tree=Try&rev=e9a7c3ae71de

Screw it, I removed the animations entirely. I don't want inconsistency, some nodes expanding with an animation, while some without. RIP, but honestly I *do* think that things feel much more sleek now.

+1 for removing animations.

One note: The window object will have lots more (possibly non-enumerable, but maybe enumerable; spec is not clear so far) properties on real-life pages on its prototype chain once the global scope polluter properly responds to enumeration. In particular, every element with an id will be a property on the window. On something like http://www.whatwg.org/specs/web-apps/current-work/ as of today that's about 6900 properties in addition to the ones that are there by default and the ones put on there by the various scripts declaring functions, etc...

(In reply to Boris Zbarsky (:bz) from comment #5) This is somewhat bad news. Aside from the variables view sluggishness, which can more or less be dealt with, one of the other bottlenecks is now with stepping. The current implementation always fetches all the prototype and properties for each scope (be it local, function, window..) every time we're stepping in the debugger. Having to wait a minute when stepping between two lines a code is less than optimal ux. I can modify the current implementation to lazily fetch everything. This will remove the quite long unresponsive pauses when stepping. However, the issue of protocol overhead remains. Retrieving a json of 6900 properties is massive. We've been dealing with much lower numbers so far (70±). I don't think we should do anything about it yet, but paging the prototype and properties response may be a good idea, just like we do with stack frames. We'll see.

This massively helps responsiveness when stepping. Every scope (except the innermost) now fetches the prototype and properties lazily. The non-enumerables container inherits the draw dispatch behavior from part1.

Drat, the patches from bug 807222 don't apply cleanly to fx-team and these don't apply cleanly to m-i...

(In reply to Panos Astithas [:past] from comment #8) > Drat, the patches from bug 807222 don't apply cleanly to fx-team and these > don't apply cleanly to m-i... That's weird. My queue is part 1-5 from 807222, then part 1-2 in here on fx-team. The main patch (part 5) in bug 807222 got backed out anyway, so feel free to postpone this review. I'll rebase this to work directly on fx-team once m-i merges over and we merge m-c, since the debugger related patch remained landed. You won't notice any slowdowns nor speedups though, until part 5 lands again, since we're getting the same number of properties at this point :)

Bug 807222 relanded, along with part5, and got merged to m-c and fx-team. Queue: http://pastebin.mozilla.org/1918872

Found a few additional optimizations that may shave a few more ms for each variable/property, like delaying the creation of tooltips, relaxing tree searching paths and simplifying a few css rules. Also added 3 more tests just to make sure things are still working properly.

Comment on attachment 680381 [details] [diff] [review] v3 Review of attachment 680381 [details] [diff] [review]: ----------------------------------------------------------------- This is a nice refactoring and although I can't say that the performance improvements are very noticeable on my high-end computer, I think we should get them. Moreover, we could easily get even better by disabling the display of non-enumerables by default. It may be a change from the current default, but I don't believe it is that big of a loss, and it seems to make an big improvement to perceived speed: while debugging Firefox for Android over USB on a debug build with every option in the gear menu turned off, I get smooth frontend updates with the stepping keyboard shortcut constantly pressed. ::: browser/devtools/debugger/debugger-controller.js @@ +726,3 @@ > } > > + aVar._retrieved = true; I don't think I get the point of setting 'fetched' early and adding a '_retrieved' property, which seems to serve the same purpose, that is set later. Can you elaborate? ::: browser/devtools/shared/VariablesView.jsm @@ +222,5 @@ > + scope.expand(); > + // fall through > + case null: > + scope._performSearch(""); > + break; What is the use case for a null or empty query string? Just expanding all scopes? @@ +1458,5 @@ > +// It would be a bad idea to re-expand them or perform expensive operations. > +VariablesView.prototype.commitHierarchyIgnoredItems = Object.create(null, { > + "window": { value: true }, > + "this": { value: true } > +}); Shouldn't we do this for the global scope as well? ::: browser/themes/gnomestripe/devtools/debugger.css @@ +210,5 @@ > -moz-margin-start: 1px; > -moz-margin-end: 1px; > transition: background 1s ease-in-out; > background: #fff; > + border-bottom: 1px solid #eee; I think I like it that it's now slightly lighter. It contrasts better with the underlined variable names. @@ +321,5 @@ > + * Variables and properties editing > + */ > + > +#variables .element-input { > + -moz-margin-start: 5px !important; Why is the important flag necessary?

(In reply to Panos Astithas [:past] from comment #12) > Comment on attachment 680381 [details] [diff] [review] > v3 > > Review of attachment 680381 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is a nice refactoring and although I can't say that the performance > improvements are very noticeable on my high-end computer, I think we should > get them. The place where the improvements were most obvious (at least on my machine) were tests: try running all tests without this patch, then running them with it applied. > Moreover, we could easily get even better by disabling the display > of non-enumerables by default. It may be a change from the current default, > but I don't believe it is that big of a loss, and it seems to make an big > improvement to perceived speed: while debugging Firefox for Android over USB > on a debug build with every option in the gear menu turned off, I get smooth > frontend updates with the stepping keyboard shortcut constantly pressed. > As discussed on IRC, we should be very careful with what we decide. Some nonenumerable properties are expected to be shown in the variables view, like .length on arrays. I also think that the gear menu isn't quite an obvious place to look for a toggle, and "show hidden properties" isn't necessarily obvious to mean "nonenumerables". I filed bug 812166. > ::: browser/devtools/debugger/debugger-controller.js > @@ +726,3 @@ > > } > > > > + aVar._retrieved = true; > > I don't think I get the point of setting 'fetched' early and adding a > '_retrieved' property, which seems to serve the same purpose, that is set > later. Can you elaborate? > Having properties fetched on mouse over, suppose they don't finish being retrieved and then owner variable is clicked. It's better to have a flag disallowing such requests (retrieving the data again if the previous call hasn't finished yet). It's also a good idea to know when the data is actually available (tests actually needed this information), hence the _retrieved property. > ::: browser/devtools/shared/VariablesView.jsm > @@ +222,5 @@ > > + scope.expand(); > > + // fall through > > + case null: > > + scope._performSearch(""); > > + break; > > What is the use case for a null or empty query string? Just expanding all > scopes? > Short answer: yes. Long answer: Unhiding all the variables and properties. Suppose there's "a" in the searchbox and you press backspace, you need to unhide everything. There are some cases in which "unhide everything" and "unhide and expand everything" should be distinguishable, hence special casing null. > @@ +1458,5 @@ > > +// It would be a bad idea to re-expand them or perform expensive operations. > > +VariablesView.prototype.commitHierarchyIgnoredItems = Object.create(null, { > > + "window": { value: true }, > > + "this": { value: true } > > +}); > > Shouldn't we do this for the global scope as well? > I don't think so. I'd like the scope to get expanded if, for example, I change the value of a variable. > ::: browser/themes/gnomestripe/devtools/debugger.css > @@ +210,5 @@ > > -moz-margin-start: 1px; > > -moz-margin-end: 1px; > > transition: background 1s ease-in-out; > > background: #fff; > > + border-bottom: 1px solid #eee; > > I think I like it that it's now slightly lighter. It contrasts better with > the underlined variable names. > That, and the fact that the line is solid and not dotted, and there's no border radius. Helps with the performance, just a bit. > @@ +321,5 @@ > > + * Variables and properties editing > > + */ > > + > > +#variables .element-input { > > + -moz-margin-start: 5px !important; > > Why is the important flag necessary? Because the element-input is a textbox with the "plain" class also applied. To override the margins specified by it, we need !important.

Addressed comments and landed: https://hg.mozilla.org/integration/fx-team/rev/7b4d2dacf833