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.

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!

Comment on attachment 8969419 [details] Bug 1455437 - Inline isIterator helper in protocol.js. https://reviewboard.mozilla.org/r/238172/#review243910

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`.

New DAMP run (with only 3 runs), still reports a 5% win on RDP test: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=02b4ef773766b86e5d9f98a8655d312ba1fe5df3&newProject=try&newRevision=fc4dbb14c3f230201d15f13b16ec837d514875b9&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&showOnlyConfident=1&framework=1

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