828664 - Debugger is sad when inspecting an array of > 10k elements in a variables view

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

From IRC:

13:30 < vlad> robcee: in the debugger, expanding a very large array breaks things badly
13:30 < vlad> e.g. something with > 10000 elements

13:31 < vlad> testcase: var foo = new Uint8Array(300000); break();
13:31 < vlad> set a breakpoint at break and expand foo :p

13:36 < robcee> vlad: is that in the variables view or just in general
13:37 < vlad> didn't try in general
13:37 < robcee> like, if you're paused on a stack with one of those arrays in scope

13:37 < vlad> it started generating (in this order) indices 1, 10, 100, 1000, 10000, 10001, 10002, ... 10009, 1001, 10011, ...
13:37 < vlad> and just generated a hell of a lot of them
13:38 < robcee> oh, I wonder if it's the sort mechanism?
13:39 < robcee> that's nasty
13:39 < vlad> yeah, looks like default JS sort (which stringifies)
13:39 < vlad> but also too many indices generated for such a large array

Comment 1

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

assigning to victor since he said he'd take a look.

Filter on BLACKEAGLE.

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v1

DIBUGAR NOT SAD ANYMOR.

https://tbpl.mozilla.org/?tree=Try&rev=84bfe210bc3d

Comment 3

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

Comment on attachment 701754 [details] [diff] [review]
v1

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

looks nice. Just testing this out, but I think this'll be fine.

Comment 4

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

looking at your patch one thing, after the initial population, it takes a few seconds to expand the object after you've collapsed it so you end up hitting the twisty again to try to make it expand and then it expands and immediately collapses again.

Kind of bad usability wise. Could we give some indication that we're waiting for the thing to expand?

Comment 5

[Dave Camp (:dcamp)]

Comment on attachment 701754 [details] [diff] [review]
v1

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

::: browser/devtools/debugger/debugger-controller.js
@@ +25,5 @@
> +  "Int32Array",
> +  "Uint32Array",
> +  "Float32Array",
> +  "Float64Array"
> +];

Would it make more sense to just provide our own comparator that treats ints/indexes and strings appropriately?

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #4)
> looking at your patch one thing, after the initial population, it takes a
> few seconds to expand the object after you've collapsed it so you end up
> hitting the twisty again to try to make it expand and then it expands and
> immediately collapses again.
> 
> Kind of bad usability wise. Could we give some indication that we're waiting
> for the thing to expand?

I'll throbber it.

(In reply to Dave Camp (:dcamp) from comment #5)
> Comment on attachment 701754 [details] [diff] [review]
> v1
> 
> Review of attachment 701754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +25,5 @@
> > +  "Int32Array",
> > +  "Uint32Array",
> > +  "Float32Array",
> > +  "Float64Array"
> > +];
> 
> Would it make more sense to just provide our own comparator that treats
> ints/indexes and strings appropriately?

Maybe. Or not:
* both arrays (and sparse arrays) seem to always get sorted properly
* [].sort() is likely faster than any completely manual sorting implementation
* [].sort(comparator) is pretty effin slow
* [].sort() works properly with objects (not so much when most of the keys are numbers, but that's a remote use case)

What do you think?

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: v1.1

Has a throbber.

Comment 8

[Dave Camp (:dcamp)]

(In reply to Victor Porof [:vp] from comment #6)

> > Would it make more sense to just provide our own comparator that treats
> > ints/indexes and strings appropriately?
> 
> Maybe. Or not:
> * both arrays (and sparse arrays) seem to always get sorted properly
> * [].sort() is likely faster than any completely manual sorting
> implementation
> * [].sort(comparator) is pretty effin slow
> * [].sort() works properly with objects (not so much when most of the keys
> are numbers, but that's a remote use case)
> 
> What do you think?

Do you think it's problematic that we show the object with an order that's different than if the user were to enumerate the objects themselves?  To ask another way: do we really need to sort at all?

bz pointed out in a recent dev.platform thread:

"Basically, objects and arrays now have the same enumeration behavior, and it's numeric properties in numeric order followed by named properties in addition order.

Note that that's the behavior Chrome implements and the behavior the ES6 drafts call for."

So we could either stick to showing default enumeration/addition order or we could at least rely on the guarantee that numeric properties will be shown first and in order...

Comment 9

[Victor Porof [:vporof][:vp]]

(In reply to Dave Camp (:dcamp) from comment #8)
> 
> Do you think it's problematic that we show the object with an order that's
> different than if the user were to enumerate the objects themselves?  To ask
> another way: do we really need to sort at all?
> 

I've been wanting to disable property sorting altogether for a while. I think we should do that.

We should keep variable sorting enabled by default. A pref to disable it is available for the adventurous.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

The basic problem sorting solves is finding a needle in DOMWindow's haystack (or any other object with a giant property list, like jQuery or Sizzle). The new property filtering functionality mitigates this issue, but I'm not sure if it's intuitive or discoverable enough.

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #10)
> The basic problem sorting solves is finding a needle in DOMWindow's haystack
> (or any other object with a giant property list, like jQuery or Sizzle). The
> new property filtering functionality mitigates this issue, but I'm not sure
> if it's intuitive or discoverable enough.

That's a good point. Sorting is relevant when browsing unfamiliar objects' properties. Is there any good way of making such differentiations? I think not, unfortunately.

Comment 13

[Victor Porof [:vporof][:vp]]

Attachment: v1.2

Fixed a failing test and added a few more assertions to make sure things are still working as expected.

Kept the filtering as in the latest patch version.
Carrying over r+.

Comment 14

[Victor Porof [:vporof][:vp]]

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/013324ca3a6b

Comment 16

[mcdavis941 (sporadically reading bugmail)]

Attachment: screenshot of throbber

Just for future reference, adding a screenshot showing the location of the new throbber.