Closed
Bug 1377279
Opened 7 years ago
Closed 7 years ago
Fine-tune array built-ins
Categories
(Core :: JavaScript: Standard Library, enhancement)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(1 file, 1 obsolete file)
13.60 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
We can fine-tune some of the array built-ins to save a few ms for the common cases. The following performance improvements are possible: --- // 200 ms-> 175 ms // (Fast path for arrays in SetLengthProperty) var dt = 0; for (var i = 0; i < 1000000; ++i) { var a = [1, 2, 3]; var t = dateNow(); a.splice(0, 0); dt += (dateNow() - t); } print(dt); // 195 ms-> 170 ms // (Fast path for arrays in SetLengthProperty) var dt = 0; for (var i = 0; i < 1000000; ++i) { var a = [1, 2, 3]; var t = dateNow(); a.unshift(0); dt += (dateNow() - t); } print(dt); // 395 ms -> 330 ms // (Array instead of List when collecting array elements) function cmp(a, b) { var d = a - b; return d; } var dt = 0; for (var i = 0; i < 1000000; ++i) { var a = [1, 2, 3]; var t = dateNow(); a.sort(cmp); dt += (dateNow() - t); } print(dt); // 280 ms -> 250 ms // (Fast path for packed arrays when collecting array elements) function cmp(a, b) { return a - b; } var dt = 0; for (var i = 0; i < 1000000; ++i) { var a = [1, 2, 3]; var t = dateNow(); a.sort(cmp); dt += (dateNow() - t); } print(dt); // 135 ms -> 115 ms // (Avoiding prototype chain walk in ObjectMayHaveExtraIndexedProperties) var dt = 0; for (var i = 0; i < 1000000; ++i) { var a = ["", ""]; var t = dateNow(); a.join(""); dt += (dateNow() - t); } print(dt); // 120 ms -> 86 ms // (Avoiding prototype chain walk in ObjectMayHaveExtraIndexedProperties and no forced dense elements initialization for packed arrays) // (95 ms when only changing ArrayReverseDenseKernel) var dt = 0; for (var i = 0; i < 1000000; ++i) { var a = [1, 2, 3]; var t = dateNow(); a.reverse(); dt += (dateNow() - t); } print(dt); // 180 ms -> 145 ms // (Avoiding prototype chain walk in ObjectMayHaveExtraIndexedProperties and SetLengthProperty for arrays) // (160 ms when only changing SetLengthProperty) var dt = 0; for (var i = 0; i < 1000000; ++i) { var a = [null, null, null]; var t = dateNow(); a.shift() dt += (dateNow() - t); } print(dt); ---
Assignee | ||
Comment 1•7 years ago
|
||
Do you think these changes will help in real-world situations?
Attachment #8882366 -
Flags: feedback?(kvijayan)
Assignee | ||
Comment 2•7 years ago
|
||
Updated patch so it actually passes the tests.
Attachment #8882366 -
Attachment is obsolete: true
Attachment #8882366 -
Flags: feedback?(kvijayan)
Attachment #8884552 -
Flags: feedback?(kvijayan)
Comment 3•7 years ago
|
||
André, I think this is great, I'd be happy to review.
Comment 4•7 years ago
|
||
Yeah, all of these help and seem like reasonable fixes. Much of our improvements are going to come from a slew of small opts like this. I'll be happy to help review with jandem.
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8884552 [details] [diff] [review] bug1377279.patch I think the patch is good enough for reviewing. It basically consists of avoid shape lookups when calling SetLengthProperty with an ArrayObject and avoiding prototype walks in ObjectMayHaveExtraIndexedProperties when array methods are called with packed arrays (*).
Attachment #8884552 -
Flags: feedback?(kvijayan) → review?(kvijayan)
Comment 6•7 years ago
|
||
Comment on attachment 8884552 [details] [diff] [review] bug1377279.patch Review of attachment 8884552 [details] [diff] [review]: ----------------------------------------------------------------- Nice work! Looks good. r=me with answer to comment please. ::: js/src/builtin/Sorting.js @@ +249,5 @@ > var denseLen = 0; > > for (var i = 0; i < len; i++) { > if (i in array) > + _DefineDataProperty(denseList, denseLen++, array[i]); I don't understand the reason for this change, can you explain why it is there?
Attachment #8884552 -
Flags: review?(kvijayan) → review+
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Kannan Vijayan [:djvj] from comment #6) > ::: js/src/builtin/Sorting.js > @@ +249,5 @@ > > var denseLen = 0; > > > > for (var i = 0; i < len; i++) { > > if (i in array) > > + _DefineDataProperty(denseList, denseLen++, array[i]); > > I don't understand the reason for this change, can you explain why it is > there? Do you mean the change from List to Array for |denseList| or using _DefineDataProperty to copy the elements? It seems to be faster to use Arrays than List when the input array is small (<100), and _DefineDataProperty is needed so we don't call setters on Array.prototype or Object.prototype.
Comment 8•7 years ago
|
||
I meant the _DefineDataProperty. My bad, I saw the "new List" in the old code and my mind automatically substituted "new Array", so it looked like you were replacing "new Array" with "[]", and a SETELEM with a call to _DefineDataProperty. It makes sense now, thanks!
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Kannan Vijayan [:djvj] from comment #8) > I meant the _DefineDataProperty. My bad, I saw the "new List" in the old > code and my mind automatically substituted "new Array", so it looked like > you were replacing "new Array" with "[]", and a SETELEM with a call to > _DefineDataProperty. I'm replacing SETELEM with INITELEM [1], :jandem beefed _DefineDataProperty a bit up in bug 1344463. :-) [1] http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/js/src/frontend/BytecodeEmitter.cpp#9377-9401 > > It makes sense now, thanks! Okay, great!
Assignee | ||
Comment 10•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8205552dde4b51b48d200286e9b666a2e82e32a
Keywords: checkin-needed
Comment 11•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52da209e46fb Fine-tune array built-ins. r=djvj
Keywords: checkin-needed
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/52da209e46fb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•