Closed Bug 1455437 Opened 2 years ago Closed 2 years ago

protocol.js's isIterator helper appears in profiler results

Categories

(DevTools :: Framework, enhancement)

enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

It looks like the intermediate function `isIterator` introduces some overhead.

Simplifying its implementation doesn't help:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=d3e784a5320a0afe0159253856c8424a146f5ec9&newProject=try&newRevision=9f6ccd4c85a96047d955d1833b62f1362e35fad2&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1
 function isIterator(v) {
-  return v && typeof v === "object" && Symbol.iterator in v && !Array.isArray(v);
+  return v && typeof v.then === "function";
 }

While inlining it helps:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=9f6ccd4c85a9&newProject=try&newRevision=d23645e281b6000854043fde38e75f95011093ec&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1
-  if (isIterator(v)) {
+  if (v && typeof v.then === "function") {

With that, mechanically, we can't see isIterator function, but DAMP highlights an improvement.
Assignee: nobody → poirot.alex
Kind of sad that it's needed, but let's do it for the perf. :)
Comment on attachment 8969420 [details]
Bug 1455437 - Prevent multiple Map query by simplifying _sendEvent method.

https://reviewboard.mozilla.org/r/238174/#review243906

Nice, looks good!
Attachment #8969420 - Flags: review?(jryans) → review+
Comment on attachment 8969419 [details]
Bug 1455437 - Inline isIterator helper in protocol.js.

https://reviewboard.mozilla.org/r/238172/#review243910
Attachment #8969419 - Flags: review?(jryans) → review-
Comment on attachment 8969419 [details]
Bug 1455437 - Inline isIterator helper in protocol.js.

https://reviewboard.mozilla.org/r/238172/#review243912

::: 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 (isIterator(v)) {
> +  if (v && typeof v.then === "function") {

Is that really about iterators?  Why?

Could we instead use `Symbol.iterator`?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Comment on attachment 8969419 [details]
> Bug 1455437 - Inline isIterator helper in protocol.js.
> 
> https://reviewboard.mozilla.org/r/238172/#review243912
> 
> ::: 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 (isIterator(v)) {
> > +  if (v && typeof v.then === "function") {
> 
> Is that really about iterators?  Why?
> 
> Could we instead use `Symbol.iterator`?

I followed bug 1453881 comment 8.
Immutable is also doing this:
  https://searchfox.org/mozilla-central/source/devtools/client/shared/vendor/immutable.js#224
And picked that as it looks cheaper (less checks),
but I must be very confused because of Promise.jsm stories and mixed up the naming.

Would you be fine with the same check, but with the right naming:
  if (v && typeof v.next === "function") {
?

Another option is also to revisit this feature and throw if we don't pass a real array when we expect one.
(In reply to Alexandre Poirot [:ochameau] from comment #7)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> > Comment on attachment 8969419 [details]
> > Bug 1455437 - Inline isIterator helper in protocol.js.
> > 
> > https://reviewboard.mozilla.org/r/238172/#review243912
> > 
> > ::: 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 (isIterator(v)) {
> > > +  if (v && typeof v.then === "function") {
> > 
> > Is that really about iterators?  Why?
> > 
> > Could we instead use `Symbol.iterator`?
> 
> I followed bug 1453881 comment 8.
> Immutable is also doing this:
>  
> https://searchfox.org/mozilla-central/source/devtools/client/shared/vendor/
> immutable.js#224
> And picked that as it looks cheaper (less checks),
> but I must be very confused because of Promise.jsm stories and mixed up the
> naming.
> 
> Would you be fine with the same check, but with the right naming:
>   if (v && typeof v.next === "function") {
> ?
> 
> Another option is also to revisit this feature and throw if we don't pass a
> real array when we expect one.

Ah okay!  If you use `next`, then it seems fine to me.  I am curious how often / what methods even use this feature at all, but maybe that's for a separate bug...?
Comment on attachment 8969419 [details]
Bug 1455437 - Inline isIterator helper in protocol.js.

https://reviewboard.mozilla.org/r/238172/#review243930

r+ assuming you change to `next`.
Attachment #8969419 - Flags: review- → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d7c927bd4a2
Inline isIterator helper in protocol.js. r=jryans
https://hg.mozilla.org/integration/autoland/rev/3ab74afe2556
Prevent multiple Map query by simplifying _sendEvent method. r=jryans
https://hg.mozilla.org/mozilla-central/rev/6d7c927bd4a2
https://hg.mozilla.org/mozilla-central/rev/3ab74afe2556
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: dt-pjs
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.