Closed Bug 1454792 Opened 5 years ago Closed 5 years ago

protocol.js duplicates all arrays in identityWrite function

Categories

(DevTools :: Framework, enhancement)

Product:
DevTools
Component:
Framework
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-fission)

Attachments

(3 files)

Bug 1454792 - Stop duplicating arrays in protocol.js's identifyWrite function.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1454792 - Use arrow functions instead of bind in protocol.js.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1454792 - Prevent duplicating arrays in Array type's read/write methods.
59 bytes, text/x-review-board-request
jryans
: review+
Details 
All arrays of primitives are currently duplicated if present in request or response.
That because of `identifyWrite` method here, tries to convert iterators into arrays:
https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/devtools/shared/protocol.js#111
  // This has to handle iterator->array conversion because arrays of
  // primitive types pass through here.
  if (v && typeof (v) === "object" && Symbol.iterator in v) {
    return [...v];
  }
The issue is that Symbol.iterator is also present in arrays...
Assignee: nobody → poirot.alex
Comment on attachment 8968679 [details]
Bug 1454792 - Use arrow functions instead of bind in protocol.js.

Would you mind bundling this another cleanup in this bug?
This one doesn't report any improvement on DAMP, but I saw frames related to the writeError's bind in the profiler. Now, I hope that arrow functions aren't slower than bind?! (at least DAMP doesn't report a significant increase!)
Comment on attachment 8968681 [details]
Bug 1454792 - Prevent duplicating arrays in Array type's read/write methods.

Last patch for this bug. This time, very similar to the first one, but for arrays of non-primitive types.
No particular report from DAMP as the benchmark test doesn't involve such arrays (it probably should)
Summary: protocol.js dupliates all arrays in identityWrite function → protocol.js duplicates all arrays in identityWrite function
Comment on attachment 8968677 [details]
Bug 1454792 - Stop duplicating arrays in protocol.js's identifyWrite function.

https://reviewboard.mozilla.org/r/237348/#review243174

Good catch! :)

::: devtools/shared/protocol.js:130
(Diff revision 1)
>    if (v === undefined) {
>      throw Error("undefined passed where a value is required");
>    }
>    // This has to handle iterator->array conversion because arrays of
>    // primitive types pass through here.
> -  if (v && typeof (v) === "object" && Symbol.iterator in v) {
> +  if (v && typeof v === "object" && Symbol.iterator in v && !("length" in v)) {

Could we use `!Array.isArray(v)`?
Attachment #8968677 - Flags: review?(jryans) → review+
Comment on attachment 8968679 [details]
Bug 1454792 - Use arrow functions instead of bind in protocol.js.

https://reviewboard.mozilla.org/r/237350/#review243176
Attachment #8968679 - Flags: review?(jryans) → review+
Comment on attachment 8968681 [details]
Bug 1454792 - Prevent duplicating arrays in Array type's read/write methods.

https://reviewboard.mozilla.org/r/237356/#review243178

::: devtools/shared/protocol.js:219
(Diff revision 1)
>      return types.addType(name);
>    }
>    return types.addType(name, {
>      category: "array",
> -    read: (v, ctx) => [...v].map(i => subtype.read(i, ctx)),
> -    write: (v, ctx) => [...v].map(i => subtype.write(i, ctx))
> +    read: (v, ctx) => {
> +      if (v && typeof v === "object" && Symbol.iterator in v && !("length" in v)) {

Could this condition move to a function shared with `identifyWrite`?

Also, as with that location, what about `!Array.isArray(v)`?
Attachment #8968681 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> Comment on attachment 8968681 [details]
> Bug 1454792 - Prevent duplicating arrays in Array type's read/write methods.
> 
> https://reviewboard.mozilla.org/r/237356/#review243178
> 
> ::: devtools/shared/protocol.js:219
> (Diff revision 1)
> >      return types.addType(name);
> >    }
> >    return types.addType(name, {
> >      category: "array",
> > -    read: (v, ctx) => [...v].map(i => subtype.read(i, ctx)),
> > -    write: (v, ctx) => [...v].map(i => subtype.write(i, ctx))
> > +    read: (v, ctx) => {
> > +      if (v && typeof v === "object" && Symbol.iterator in v && !("length" in v)) {
> 
> Could this condition move to a function shared with `identifyWrite`?

Done => isIterator helper function.

> Also, as with that location, what about `!Array.isArray(v)`?

Yes, I imagine I was looking for something cheap, but DAMP reports no difference between the two versions,
so I switched to Array.isArray.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84aa9286a031
Stop duplicating arrays in protocol.js's identifyWrite function. r=jryans
https://hg.mozilla.org/integration/autoland/rev/88b340fcdc65
Use arrow functions instead of bind in protocol.js. r=jryans
https://hg.mozilla.org/integration/autoland/rev/dd66dac690ad
Prevent duplicating arrays in Array type's read/write methods. r=jryans
Blocks: 1455437
Blocks: dt-pjs
Product: Firefox → DevTools
Whiteboard: dt-fission
You need to log in before you can comment on or make changes to this bug.