Closed
Bug 911578
Opened 10 years ago
Closed 9 years ago
introduce self-hosting intrinsic IsPackedArray and use it for implementing optimized loops in array extras
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: till, Assigned: till)
References
(Blocks 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(2 files)
3.51 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
12.21 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Brought up in bug 902822 comment 15, and seems like a very good idea.
Assignee | ||
Comment 1•9 years ago
|
||
Used in the following patch
Attachment #8340399 -
Flags: review?(jdemooij)
Assignee | ||
Updated•9 years ago
|
Assignee: general → till
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Changes the following Array extras to use fast-paths for packed arrays: every some forEach map find findIndex indexOf lastIndexOf reduce reduceRight I only tested the performance of indexOf, which got about 33% faster. js- and jittests are green, try-servering here: https://tbpl.mozilla.org/?tree=Try&rev=b5fdd551b52c We have a few tests that fail if I change IsPackedArray to always return true, so I don't think we need new ones: ecma_3/Array/15.5.4.8-01.js js1_6/Array/regress-290592.js js1_6/Array/regress-386030.js
Attachment #8340403 -
Flags: review?(jdemooij)
Updated•9 years ago
|
Attachment #8340399 -
Flags: review?(jdemooij) → review+
Comment 3•9 years ago
|
||
Comment on attachment 8340403 [details] [diff] [review] Use self-hosting intrinsic isPackedArray to optimize loops in array extras Review of attachment 8340403 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/builtin/Array.js @@ +345,5 @@ > ThrowError(JSMSG_EMPTY_ARRAY_REDUCE); > var kPresent = false; > + if (isPacked) { > + accumulator = O[k++]; > + kPresent = true; You can remove this line and move |var kPresent = false;| to the else branch below. @@ +358,3 @@ > } > + if (!kPresent) > + ThrowError(JSMSG_EMPTY_ARRAY_REDUCE); Nit: indentation looks wrong. @@ +423,5 @@ > ThrowError(JSMSG_EMPTY_ARRAY_REDUCE); > var kPresent = false; > + if (isPacked) { > + accumulator = O[k--]; > + kPresent = true; Same here; can move kPresent to the else branch.
Attachment #8340403 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 4•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f86fd4fa60 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b039ed2dbac Thanks for the quick review!
Summary: introduce self-hosting intrinsic IsDenseArray and use it for implementing optimized loops in array extras → introduce self-hosting intrinsic IsPackedArray and use it for implementing optimized loops in array extras
Comment 5•9 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #2) > We have a few tests that fail if I change IsPackedArray to always return > true, so I don't think we need new ones: Maybe you do want new test case. ;-) js> [1,2,3].map(function(v, i, a) {if (i == 0) delete a[1]; return v * 2}) [2, NaN, 6] js> [1,2,3].map(function(v, i, a) {if (i == 0) delete a[1]; return v * 2}) [2, , 6] Expected: always return `[2, , 6]`
Comment 6•9 years ago
|
||
(In reply to André Bargull from comment #5) > Maybe you do want new test case. ;-) Good catch. Sorry, I should have seen this..
Updated•9 years ago
|
Flags: needinfo?(till)
Assignee | ||
Comment 7•9 years ago
|
||
Argh, *I* should've seen this! André, thanks for catching it! I've backed out all usages of IsPackedArray for loops that invoke callbacks. All told, only indexOf, lastIndexOf and the accumulator-finding loops in reduce and reduceRight contain the optimization now. :( remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9439760a6196
Flags: needinfo?(till)
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d9f86fd4fa60 https://hg.mozilla.org/mozilla-central/rev/7b039ed2dbac https://hg.mozilla.org/mozilla-central/rev/9439760a6196
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•9 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•