Closed
Bug 1455437
Opened 6 years ago
Closed 6 years ago
protocol.js's isIterator helper appears in profiler results
Categories
(DevTools :: Framework, enhancement)
DevTools
Framework
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 | ||
Updated•6 years ago
|
Assignee: nobody → poirot.alex
Kind of sad that it's needed, but let's do it for the perf. :)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
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 5•6 years ago
|
||
mozreview-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 6•6 years ago
|
||
mozreview-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`?
Assignee | ||
Comment 7•6 years ago
|
||
(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 9•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
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
Comment 13•6 years ago
|
||
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
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d7c927bd4a2 https://hg.mozilla.org/mozilla-central/rev/3ab74afe2556
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•