Array#splice 40% slowdown
Reported by
woxxom@gmail.com,
Apr 21 2018
|
|||||||||||
Issue descriptionChrome 67, 68 Windows 7 ===================================== 1. Open test.html in Chrome 66 2. Open test.html in Chrome 67 or 68 Expected: similar times are displayed (70ms here) Observed: Chrome 67/68 time is ~40% slower (100ms here) ===================================== Bisected to 330a37f8e0df398bb0f83128d95dff7c8dec431e "Update V8 to version 6.7.12" Landed in 67.0.3364.0 V8 log: https://chromium.googlesource.com/v8/v8/+log/00dbe028..f4c40134?pretty=fuller Probable suspects: d438101070c2923ca60abb53e4f701ee05468626 "[object-stats] Improve FixedArray classification" bc72a83192c9ff9271dcdcb1a99d77de4d6357b5 "[csa] Make use of CallBuiltin (rather than CallStub) in more places" ===================================== Also note, Chrome 64-bit is 30-60% slower than 32-bit in this test.
,
Apr 23 2018
Able to reproduce this issue on latest dev 67.0.3396.10 and on latest canary 68.0.3404.0 using Mac 10.13.3, Ubuntu 14.04 and Windows 7. In Win-7 67.0.3362.0 observed 103ms but in 67.0.3364.0 observed 145ms. Good Build: 67.0.3362.0 Bad Build: 67.0.3364.0 As per comment#0 assigning to cbruni@ from https://chromium-review.googlesource.com/948502. And cc'ing neis@ from https://chromium-review.googlesource.com/936624 for further inputs on this issue. Adding RB-Stable for M-67. Please remove if not the case. Thanks!
,
Apr 24 2018
I can make the described SMI case a factor 3 faster, still investigating the slow-down effects most probably caused by GC maintenance work.
,
Apr 24 2018
This regression is caused by the following CL: 88062a2 Reland [in-place weak refs] Add in-place weak references & migrate one WeakCell to it. by Marja Hölttä · 7 weeks ago
,
Apr 24 2018
Minimal repro:
var arr = new Array(1e6).fill(undefined);
var t0 = performance.now();
for (var i = 0; i < 500; i++) {
arr.splice(0, 1);
}
console.log((performance.now() - t0) + 'ms');
,
Apr 24 2018
Assigning back to me since I have a fix in flight.
,
Apr 25 2018
M67 Stable promotion is coming soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Apr 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/e570e67383577c7f5ab6da7beb68631bab4ba75d commit e570e67383577c7f5ab6da7beb68631bab4ba75d Author: Camillo Bruni <cbruni@chromium.org> Date: Wed Apr 25 17:00:46 2018 [heap][elements] Improve Array.prototype.splice speed - 30% speedup by adding HeapObject shortcut for Heap::InNewSpace Bug: chromium:835558 Change-Id: I48b5ec43a5ecdd7d82827c955ab418fdeff449d8 Reviewed-on: https://chromium-review.googlesource.com/1027471 Commit-Queue: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Reviewed-by: Marja Hölttä <marja@chromium.org> Cr-Commit-Position: refs/heads/master@{#52790} [modify] https://crrev.com/e570e67383577c7f5ab6da7beb68631bab4ba75d/src/heap/heap-inl.h [modify] https://crrev.com/e570e67383577c7f5ab6da7beb68631bab4ba75d/src/heap/heap.h [modify] https://crrev.com/e570e67383577c7f5ab6da7beb68631bab4ba75d/src/objects-inl.h [modify] https://crrev.com/e570e67383577c7f5ab6da7beb68631bab4ba75d/src/objects.h
,
Apr 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/dcdabdc86ada34eeb51af0723c90112cddd54643 commit dcdabdc86ada34eeb51af0723c90112cddd54643 Author: Camillo Bruni <cbruni@chromium.org> Date: Wed Apr 25 17:41:31 2018 [elements] Improve Array.prototype.splice speed By using memmove for SMI elements we get a roughly 3x speedup over the slower iterative copying with write barriers. Bug: chromium:835558 Change-Id: I73da07a1648a3495ff78212ffa1ed949d205a7d2 Reviewed-on: https://chromium-review.googlesource.com/1028236 Reviewed-by: Igor Sheludko <ishell@chromium.org> Commit-Queue: Camillo Bruni <cbruni@chromium.org> Cr-Commit-Position: refs/heads/master@{#52792} [modify] https://crrev.com/dcdabdc86ada34eeb51af0723c90112cddd54643/src/elements.cc
,
Apr 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/9a7c4bfe1e8f7c917acb0b302be299fccafebb16 commit 9a7c4bfe1e8f7c917acb0b302be299fccafebb16 Author: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Date: Wed Apr 25 19:46:50 2018 Revert "[elements] Improve Array.prototype.splice speed" This reverts commit dcdabdc86ada34eeb51af0723c90112cddd54643. Reason for revert: broke tsan. Original change's description: > [elements] Improve Array.prototype.splice speed > > By using memmove for SMI elements we get a roughly 3x speedup over the slower > iterative copying with write barriers. > > Bug: chromium:835558 > Change-Id: I73da07a1648a3495ff78212ffa1ed949d205a7d2 > Reviewed-on: https://chromium-review.googlesource.com/1028236 > Reviewed-by: Igor Sheludko <ishell@chromium.org> > Commit-Queue: Camillo Bruni <cbruni@chromium.org> > Cr-Commit-Position: refs/heads/master@{#52792} TBR=cbruni@chromium.org,ishell@chromium.org Change-Id: I77c46fe3d47d651de3c39df9fbf5f30c340188e2 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:835558 Reviewed-on: https://chromium-review.googlesource.com/1028337 Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Cr-Commit-Position: refs/heads/master@{#52795} [modify] https://crrev.com/9a7c4bfe1e8f7c917acb0b302be299fccafebb16/src/elements.cc
,
Apr 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/a3c48cf2c983eaef6733827e95a2e4944abecd56 commit a3c48cf2c983eaef6733827e95a2e4944abecd56 Author: Camillo Bruni <cbruni@chromium.org> Date: Fri Apr 27 17:04:26 2018 Reland "[elements] Improve Array.prototype.splice speed" This reverts commit 9a7c4bfe1e8f7c917acb0b302be299fccafebb16. Reason for revert: <INSERT REASONING HERE> Original change's description: > Revert "[elements] Improve Array.prototype.splice speed" > > This reverts commit dcdabdc86ada34eeb51af0723c90112cddd54643. > > Reason for revert: broke tsan. > > Original change's description: > > [elements] Improve Array.prototype.splice speed > > > > By using memmove for SMI elements we get a roughly 3x speedup over the slower > > iterative copying with write barriers. > > > > Bug: chromium:835558 > > Change-Id: I73da07a1648a3495ff78212ffa1ed949d205a7d2 > > Reviewed-on: https://chromium-review.googlesource.com/1028236 > > Reviewed-by: Igor Sheludko <ishell@chromium.org> > > Commit-Queue: Camillo Bruni <cbruni@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#52792} > > TBR=cbruni@chromium.org,ishell@chromium.org > > Change-Id: I77c46fe3d47d651de3c39df9fbf5f30c340188e2 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: chromium:835558 > Reviewed-on: https://chromium-review.googlesource.com/1028337 > Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> > Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> > Cr-Commit-Position: refs/heads/master@{#52795} TBR=kozyatinskiy@chromium.org,cbruni@chromium.org,ishell@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: chromium:835558 Change-Id: I57aedb3536b81c97cf4e7ab6d863aa1dc24c20b4 Reviewed-on: https://chromium-review.googlesource.com/1032743 Reviewed-by: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Reviewed-by: Igor Sheludko <ishell@chromium.org> Commit-Queue: Camillo Bruni <cbruni@chromium.org> Cr-Commit-Position: refs/heads/master@{#52857} [modify] https://crrev.com/a3c48cf2c983eaef6733827e95a2e4944abecd56/src/elements.cc [modify] https://crrev.com/a3c48cf2c983eaef6733827e95a2e4944abecd56/src/heap/heap.cc [modify] https://crrev.com/a3c48cf2c983eaef6733827e95a2e4944abecd56/src/heap/heap.h
,
May 2 2018
,
May 2 2018
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 2 2018
There are multiple CLs listed here. Could you pls clarify which CLs you're requesting a merge for ? And how safe they are?
,
May 2 2018
,
May 2 2018
*** Bulk Edit *** M67 Stable promotion is coming soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. If fix is already merged to M67 and nothing else is pending, pls mark the bug as fixed. Thank you.
,
May 3 2018
Given that only one of the two fixes addresses the regression I would only backmerge that one (the other CL is an improvement on top of that): InNewSpace Fix commit e570e67383577c7f5ab6da7beb68631bab4ba75d
,
May 3 2018
This is an easy fix with low risk.
,
May 3 2018
Approving merge for e570e67383577c7f5ab6da7beb68631bab4ba75d to M67 branch 3396 based on comments #17 and #18. Pls merge ASAP. Als pls mark bug as fixed after the merge if nothing else is pending. Thank you.
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/0b8a5ec2837a008ba96e541551d027aee42b73e2 commit 0b8a5ec2837a008ba96e541551d027aee42b73e2 Author: Camillo Bruni <cbruni@chromium.org> Date: Thu May 03 09:52:35 2018 Merged: [heap][elements] Improve Array.prototype.splice speed Revision: e570e67383577c7f5ab6da7beb68631bab4ba75d BUG= chromium:835558 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=mlippautz@chromium.org Change-Id: Ic6e575edc4bb0a46f1c292baa3331b8c93d8b34a Reviewed-on: https://chromium-review.googlesource.com/1041866 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/branch-heads/6.7@{#51} Cr-Branched-From: 8457e810efd34381448d51d93f50079cf1f6a812-refs/heads/6.7.288@{#2} Cr-Branched-From: e921be5c4f2c6407936bde750992dedbf47c1016-refs/heads/master@{#52547} [modify] https://crrev.com/0b8a5ec2837a008ba96e541551d027aee42b73e2/src/heap/heap-inl.h [modify] https://crrev.com/0b8a5ec2837a008ba96e541551d027aee42b73e2/src/heap/heap.h
,
May 3 2018
,
May 3 2018
Able to reproduce this issue on reported version using Mac 10.13.3, Ubuntu 17.10 and Windows 10. Hence verifying the fix on latest canary 68.0.3418.0. Observing very less 66ms only now. Attaching screenshot for reference. As fix is working as expected adding TE-Verified labels. Thanks!
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/c0035a4f30510b468501f6f36c094254470c1c61 commit c0035a4f30510b468501f6f36c094254470c1c61 Author: Camillo Bruni <cbruni@chromium.org> Date: Thu May 03 18:08:42 2018 [verify-heap] Improve elements verification This is a preparatory CL to find a potential regression on x86. Bug: chromium:835558 Change-Id: I3859b59d1497d4b7447ad38ee352cf4bbdeb4502 Reviewed-on: https://chromium-review.googlesource.com/1027842 Commit-Queue: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Igor Sheludko <ishell@chromium.org> Cr-Commit-Position: refs/heads/master@{#52967} [modify] https://crrev.com/c0035a4f30510b468501f6f36c094254470c1c61/src/objects-debug.cc
,
May 3 2018
Is cl listed at #23 need a merge to M67?
,
May 4 2018
No, CL #23 is an independent CL to address some issues found with the CL from #9 (which have been fixed in the mean time). |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by susan.boorgula@chromium.org
, Apr 22 2018