This is bug 1023386 again. STR: Execute this in the web console: inspect(new Uint8Array(new ArrayBuffer(2354207))) Current behavior: Takes about 10s to open the variable view. Expected behavior: Array is displayed immediately.

Various tweaks need to be made to the property iterator...

With green tests and cleanup around iterator actors.

Comment on attachment 8750762 [details] [diff] [review] patch v2 Review of attachment 8750762 [details] [diff] [review]: ----------------------------------------------------------------- Jarda, you worked on Maps and Sets. I think you would have a valuable feedback on this patch. I'm trying to unify all these iterator actors in order to make them all efficient. For now, it is slow for everything: typed array, but also big arrays, sets, maps and objects... For example, in map and sets, we do compute ownProperties for all entries, whereas the PropertyIterator, by design, will lazily fetch entries by chunks < 2000. It takes an unecessary time and memory to compute all values upfront. On really big objects, it just make firefox freeze and crash on OOM. With this patch I focus on computing less things upfront. Just the key names and count. Then we lazily fetch values.

Comment on attachment 8750762 [details] [diff] [review] patch v2 Hello Alex, this lazy property-fetching is a good optimization! Until now, the PropertyIteratorActor only optimized the volume of data transferred over the RDP protocol, but server-side, all property values were always enumerated upfront. Before it lands, I think the following changes would make the code much cleaner: The ugly code that does the unsafe dereferences and Xrays waiving should live only at one place, inside the enum*Entries functions. In addition to the iterator interface, the object they return should get methods for random access. Then they will have the API necessary for both object previews and serving the enumProperties/Entries actor methods. If the enum*Entries functions have the random access API, you can get rid of the PropertyIteratorActor inheritance. All the subclass-specific behavior will be encapsulated in the object returned by enum*Entries. You can do "this.enumerator = enum*Entries()" in the PropertyIteratorActor and then call this.enumerator methods. Replace inheritance with composition. I don't understand what's the purpose of the skipObject parameter of _findSafeGetterValues. Is it guaranteed that the object will never have any safe (i.e., native) getters and that they are always only on the prototype? Does the skipping make enumerating large objects and arrays faster? Shouldn't _findSafeGetterValues be called with skipObject = true also in the GenericObject function, which generates the object preview?

(In reply to Jarda Snajdr [:jsnajdr] from comment #8) > Comment on attachment 8750762 [details] [diff] [review] > patch v2 > > Hello Alex, this lazy property-fetching is a good optimization! Until now, > the PropertyIteratorActor only optimized the volume of data transferred over > the RDP protocol, but server-side, all property values were always > enumerated upfront. > > Before it lands, I think the following changes would make the code much > cleaner: > The ugly code that does the unsafe dereferences and Xrays waiving should > live only at one place, inside the enum*Entries functions. In addition to > the iterator interface, the object they return should get methods for random > access. Then they will have the API necessary for both object previews and > serving the enumProperties/Entries actor methods. Here, by "random", you mean "lazy", right? Otherwise I'm a bit confused. > I don't understand what's the purpose of the skipObject parameter of > _findSafeGetterValues. Is it guaranteed that the object will never have any > safe (i.e., native) getters and that they are always only on the prototype? That's my comprehension of this code. But TBH, I don't fully understood it at first when I introduced the PropertyIterator. Look into findSafeGetterValues() and findSafeGetters(). It ends up using object.getOwnPropretyNames() which we are using in PropertyIterator when we don't list only indexed properties. So my current comprehension is that we just end up calling object.getOwnPropertyNames() twice on the object for nothing. And getOwnPropertyNames returns all array items, so it expensive on very big arrays. > Does the skipping make enumerating large objects and arrays faster? Yes, a lot! That's a key optimization. For big arrays, we should avoid calling getOwnPropertyNames. Calling it is somewhat ok, but we shouldn't do any processing on it, like the various filter/sort we may do in _initProperties. > Shouldn't _findSafeGetterValues be called with skipObject = true also in the > GenericObject function, which generates the object preview? I have no idea. GenericObject seems to also call getOwnPropertyName by itself. But it also returns safeGetterValues as-is. But it pass it's already known list of properties and seems to filter them. That safe getter is just a mess. I don't know what the client really expects.

(In reply to Alexandre Poirot [:ochameau] from comment #9) > Here, by "random", you mean "lazy", right? Otherwise I'm a bit confused. No, I really mean "random" :) The enumMapEntries function should look like this: function enumMapEntries(objectActor) { ... extract stuff from the objectActor return { [Symbol.iterator]: function* () { ... iterate over entries (sequential access) } get count() { ... return entries count } propertyName(idx) { ... property name @idx (random access) } propertyValue(idx) { ... property value @idx (random access) } } } Then the returned object encapsulates all differences between various types (object, array, map, ...) and can be used by a generic PropertyIteratorActor without any subclassing. And also by the ObjectActorPreviewers, without any code duplication. Currently, only the [Symbol.iterator] is supported, which is not enough if we want to fetch values in slices and lazily. > > > I don't understand what's the purpose of the skipObject parameter of > > _findSafeGetterValues. Is it guaranteed that the object will never have any > > safe (i.e., native) getters and that they are always only on the prototype? > > That's my comprehension of this code. But TBH, I don't fully understood it > at first when I introduced the PropertyIterator. > Look into findSafeGetterValues() and findSafeGetters(). It ends up using > object.getOwnPropretyNames() which we are using in PropertyIterator when we > don't list only indexed properties. > So my current comprehension is that we just end up calling > object.getOwnPropertyNames() twice on the object for nothing. And > getOwnPropertyNames returns all array items, so it expensive on very big > arrays. The point of the "safe getters" is to show the values of dynamic properties that have a "getter" defined, but not if it would involve calling the debuggee's JS code. That's unsafe and unwanted, and will soon start throwing errors (DebuggeeWouldRun). But we want to display properties of native objects, mostly DOM, like mouse event's x,y coordinates etc. If your patch doesn't break this (i.e., the native properties are defined on the prototype), then it's probably fine. > > Shouldn't _findSafeGetterValues be called with skipObject = true also in the > > GenericObject function, which generates the object preview? > > I have no idea. GenericObject seems to also call getOwnPropertyName by > itself. But it also returns safeGetterValues as-is. But it pass it's already > known list of properties and seems to filter them. That safe getter is just > a mess. I don't know what the client really expects. Both PropertyIteratorActor and ObjectActorPreviewers should expose the same information (the object properties) to two different APIs. The retrieval of this information should be therefore shared code. This is done correctly for Maps and Sets, using the enum*Entries functions, but it's a mess for Objects and Arrays - here are two implementations that are very similar, but not identical. Should be merged together.

This patch is now using composition instead of inheritance. I introduced enum functions for arrays and objects. There is just this safe getter story which I'm unsure of...

(In reply to Alexandre Poirot [:ochameau] from comment #11) > There is just this safe getter story which I'm unsure of... Someone with detailed knowledge of the Debugger API should be able to help with this. jimb, fitzgen, ejpbruel...

This patch is only about the property iterator tweaks. Jarda, you are the best qualified to look at this patch, do you mind reviewing it? I would then ask victor to rubber stamp it.

Remove unecessary ignoreSafeGetters option of enumProperties request And prevent looking for safe getters on objects with many properties that only have some on their prototype Here is a small snippet that proves there is no safe getter to look for on most objects: let objectsWoSafeGetters = [[], {}, new Date(), /regexp/, "string", function () {}, true, 12, new Set(), new WeakSet(), new WeakMap(), new Map(), new Error(), new Promise(()=>{})]; objectsWoSafeGetters.map(o => Object.getOwnPropertyNames(o).some(n => {return !!Object.getOwnPropertyDescriptor(o, n).get;})) I've only found window and document that have some: let objectsWithSafeGetters = [window, document]; objectsWithSafeGetters.map(o => Object.getOwnPropertyNames(o).some(n => {return !!Object.getOwnPropertyDescriptor(o, n).get;})) navigator, document.body (so elements) don't have. So, most objects don't have safe getter on them. But most have some being inherited from their prototype, so we only need to skip looking for some on the object, but still go through the prototype chain. I do prevent looking for safe getters on arrays, typed arrays and objects as it is very slow, (and useless) on the one with many items or properties!

Comment on attachment 8753774 [details] [diff] [review] safe getters tweaks - v1 Review of attachment 8753774 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments addressed. Thanks Alex! ::: devtools/server/actors/object.js @@ +278,5 @@ > let safeGetterValues = Object.create(null); > let obj = this.obj; > let level = 0, i = 0; > > + // Most object don't have any safe getter but inherit some from their "Most objects don't have any safe getters" @@ +280,5 @@ > let level = 0, i = 0; > > + // Most object don't have any safe getter but inherit some from their > + // prototype. Avoid calling getOwnPropertyNames on objects that may have > + // many properties like Array, strings or js objects. Please add the fact that this is intended as a performance optimisation to the comment. In general, comments should explain why code is doing something, since one can already infer what the code is doing from the code itself.

Comment on attachment 8753772 [details] [diff] [review] patch v4 Review of attachment 8753772 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/object.js @@ +722,5 @@ > * Regarding value filtering it just compare to the stringification > * of the property value. > */ > function PropertyIteratorActor(objectActor, options) { > + if (options.ignoreNonIndexedProperties) { nit: I'd put this condition at the end, so that enumArrayProperties and enumObjectProperties are close together. @@ +788,2 @@ > > +function enumArrayProperties(objectActor) { enumArrayProperties doesn't support options.query, so searching in variables won't find array values. I realize it's an omission in Map/Set entries code, too. @@ +802,5 @@ > > + return { > + size: length, > + propertyName(index) { > + return index; How is this going to work for pseudoarrays with string properties? E.g. { "0":"a", "1":"b" }? Will indexed/non-indexed properties work for them correctly? @@ +817,5 @@ > + try { > + // getOwnPropertyNames returns only indexed properties for typed arrays. > + // safe getter are going to retrieve non-indexed properties via prototype > + // chain. > + if (isTypedArray && options.ignoreIndexedProperties) { But I can set a property directly on a typed array, can't I? let a = new UInt32Array(); a.prop = "hello"; console.log(a.prop); // logs "hello" for me. @@ +848,3 @@ > } > > + if (options.ignoreIndexedProperties) { This condition is redundant here. @@ +927,5 @@ > // same reason we do so for Arrays above - this filtering behavior is likely > // to be more confusing than beneficial in the case of Object previews. > let raw = objectActor.obj.unsafeDereference(); > > + let entries = [...Cu.waiveXrays(Map.prototype.entries.call(raw))]; Maybe we could lazily fetch the map values, too. Call only Map.prototype.keys here and then fetch the value with Map.prototype.get in propertyDescription? @@ +940,5 @@ > + return index; > + }, > + propertyDescription(index) { > + let key = entries[index][0]; > + let val = entries[index][1]; nit: let [key, val] = entries[index]

Comment on attachment 8753772 [details] [diff] [review] patch v4 Review of attachment 8753772 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/object.js @@ +788,4 @@ > > +function enumArrayProperties(objectActor) { > + let length = DevToolsUtils.getProperty(objectActor.obj, "length"); > + if (typeof(length) !== "number") { ESLint reports many errors in this file. For example, "typeof(length)" is frowned upon, in favor of "typeof length" (unary operator like new or delete). @@ +868,5 @@ > + } > + // and then on attribute values > + let desc; > + try { > + desc = obj.getOwnPropertyDescriptor(name); "obj" is undefined here. Should be "objectActor.obj".

Here is an interdiff on top of attachment 8753772 [details] [diff] [review].

See the interdiff in attachment 8754565 [details] [diff] [review] for all new changes. You were right about custom properties on typed array and query no longer working on arrays, I fixed all that. Search will be still slow on very large arrays, but there isn't much way to optimize that. I also fixed various eslint issues. (In reply to Jarda Snajdr [:jsnajdr] from comment #17) > @@ +802,5 @@ > > > > + return { > > + size: length, > > + propertyName(index) { > > + return index; > > How is this going to work for pseudoarrays with string properties? E.g. { > "0":"a", "1":"b" }? Will indexed/non-indexed properties work for them > correctly? So. this is a bit complex. The client is going to spawn two iterators. A first one for indexed properties. So it is going through this code. And another one for non-indexed properties, going through enumObjectIterator. Now, if a query is specified, it will spawn two enumObjectIterator. > @@ +927,5 @@ > > // same reason we do so for Arrays above - this filtering behavior is likely > > // to be more confusing than beneficial in the case of Object previews. > > let raw = objectActor.obj.unsafeDereference(); > > > > + let entries = [...Cu.waiveXrays(Map.prototype.entries.call(raw))]; > > Maybe we could lazily fetch the map values, too. Call only > Map.prototype.keys here and then fetch the value with Map.prototype.get in > propertyDescription? I realize I forgot to address this, will do in the next round. > @@ +940,5 @@ > > + return index; > > + }, > > + propertyDescription(index) { > > + let key = entries[index][0]; > > + let val = entries[index][1]; > > nit: let [key, val] = entries[index] Same for this one.

Comment on attachment 8754568 [details] [diff] [review] patch v5 Review of attachment 8754568 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Possible followups: - improve search performance (bug 897227) - lazy value fetching for Maps - unify enum(Array|Object)Properties with GenericObject and Array previewers I'd also check if the changes in this bug are sufficiently covered by tests. For example, would mochitest catch the missing typed array properties? Or the broken options.query on arrays and objects?

Fix the two leftovers from comment 20. Merged the two patches and rebased. I tried to cover custom properties on typed array, but that doesn't fit existing tests. I'll try to come up with a new dedicated test in a followup.

https://hg.mozilla.org/integration/fx-team/rev/73912a342d3731b8e292229fa940b623a8719d7b Bug 1270179 - Improve performances when inspecting big typed arrays in the console. r=jsnajdr,ejpbruel

I have reproduced this bug with Firefox nightly 49.0a1(build id:20160504030302)on windows 7(64 bit) I have verified this bug as fixed with Firefox beta 49.0b7(build id:20160825132718) User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 [bugday-20160907]