Significant javascript performance regression
Reported by
eno...@onshape.com,
Apr 24 2016
|
||||||||||||||||
Issue descriptionChrome Version : 51.0.2704.22 (Official Build) beta (64-bit) URLs (if applicable) : https://cad.onshape.com/documents/b403b522fa3c444f0efc8567/w/88b86cc240a19e9db587eb1b/e/bd664d4588c05c1c5e8daff9 Other browsers tested: Add OK or FAIL, along with the version, after other browsers where you have tested this issue: Chrome 49.0.2623.112 (Official Build) (64-bit): OK What steps will reproduce the problem? (1) Navigate to https://cad.onshape.com/documents/b403b522fa3c444f0efc8567/w/88b86cc240a19e9db587eb1b/e/bd664d4588c05c1c5e8daff9 (2) Wait for page to load What is the expected result? In Chrome 49, the page will take about 30-40 seconds to fully load (depending on bandwidth). The page is fully loaded when the 3D model has been rendered. What happens instead? In Chrome 51 the page can take upwards of 90 seconds to fully load. The Chrome profiler reveals that a substantial amount of time is being spent in a method that is utilized heavily for laying out the model's vertex buffer. In Chrome 49 the method takes next to no time to complete. In Chrome 51, each execution of the method can take 10s of milliseconds. In aggregate, this makes up the 60 second difference between 49 and 51. Please provide any additional information below. Attach a screenshot if possible. The method in question is a binary search insertion routine that inserts an array into a sorted array. During one profiling run, the method was noted to be de-optimized because it was "Optimized too many times". However, I do not consistently receive this warning in the profiler. There are times when the performance is similar to Chrome 49. I've found that when reloading the page the execution is faster than when I navigate from another page. However, that may be coincidence. The performance in Chrome 49 is consistently better. Though I took timings in Chrome 51, the regressed behavior also appears in Chrome 50.
,
Apr 25 2016
Able to reproduce this on the latest canary(52.0.2715.0), stable(50.0.2661.86/87) on Windows-7, Mac OS 10.11.3 and Linux Ubuntu 14.04. Test steps followed: ===================== 1. Opened incognito mode on both, tested build and the chrome canary version:52.0.2715.0 2. Navigated to the test URL on both tested build and chrome canary and observed the time difference b/w the complete rendering of the page. 3. Once the pages are loaded on both tested and canary version, refreshed the page and observed the time difference again for complete page rendering. Based on the above test steps: Last good build: 50.0.2657.0 First bad build: 50.0.2658.0 Changelog: ========== https://chromium.googlesource.com/chromium/src/+log/3cf850241bbfa1787af40672c9a57d9837205ed9..b53bed5141561f3b6b41e1f8f6f36375b3922f35 V8-changelog: ============== https://chromium.googlesource.com/v8/v8/+log/6d64dbb0..d6e3cfd0 Suspecting:https://codereview.chromium.org/1689733002. littledan@: Could you please take a look and help in investigating this further. Note: Marking this as RB-Stable for M-50 for next stable refresh if there is any. Feel free to adjust if someone feels otherwise. Thank you!
,
Apr 25 2016
I believe the linked patch was shipped as on only in 51, not in 50. What makes you suspect that patch?
,
Apr 25 2016
This range is shipped to Chrome 50: https://chromium.googlesource.com/v8/v8/+log/branch-heads/5.0
,
Apr 25 2016
Yes, the patch is shipped, but it is supposed to be turned off by a flag. During development, I ran performance tests to verify that, when the flag is turned off, certain microbenchmarks that I knew would worsen were not affected by the flag. It's possible I missed something; I'm just wondering what evidence led that patch to be suspected; this could help me in my investigation.
,
Apr 25 2016
Oh, I see--there's basically nothing else in that range. Well, we could hypothetically revert this patch off in the 50 branch, if we want a backport. It's not doing anything there, since it is turned off until 51. Then debug at a slightly more relaxed pace while 51 is in progress.
,
Apr 25 2016
Adding a few startup people. Probably it makes sense to add something similar as a benchmark? It does not show up on speedometer: https://chromeperf.appspot.com/report?sid=f072c934b94c340f82019315a228ad7ed3dd8c17df4343a871f62df0ec2b074b&start_rev=376492&end_rev=378574 https://chromium.googlesource.com/chromium/src/+/5cee23daac4df584a70ff2939715d44e008cf3e2 is the roll btw.
,
Apr 25 2016
I'm wondering: will this be fixed in an update to 50? (It seems like it might be, but I'm not familiar with the meaning by behind the different bug tags, like ReleaseBlock.) If you identify the root cause and have an idea for a work-around, please let me know.
,
Apr 25 2016
#6: It should be easily possible to bisect the regression into the small range and verify that it is the culprit. I think it is suspicious that the code shouldn't even be active in M50 but it is. This means "blindly" reverting it might have a higher risk of negative side-effects. I would propose investigating this for maximum a day. Cbruni's work regarding runtime metrics might come in handy. I think he even ported them to M50 already.
,
Apr 25 2016
,
Apr 25 2016
Ad #8: Yes, the plan is to fix this in M50 too.
,
Apr 26 2016
I've verified locally that the bisect identifies the range properly for a slowdown--Chrome is fast before and slow after. I've also identified a place in the code that might be the culprit: the species patch includes some writes and reads of the species protector that take place even if --harmony-species is off. I wrote a quick patch to fix that issue, at https://codereview.chromium.org/1918383002 . I have not yet verified whether it addresses the performance degradation.
,
Apr 26 2016
On my machine, load time of that patch goes from ~70 seconds to ~30 seconds with Dan's patch, so that's promising as a merge candidate for M50. It's still worrying to me that the protector has that level of overhead, though, as the flag is enabled in M51.
,
Apr 26 2016
Thanks for the help testing, Adam. Yes, this definitely needs a different fix for M51.
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/b1ec4cb67b7cc41be487b6780fb5c5058901ff9a commit b1ec4cb67b7cc41be487b6780fb5c5058901ff9a Author: littledan <littledan@chromium.org> Date: Tue Apr 26 19:11:41 2016 Do not penalize performance when --harmony-species is off This patch ensures that the species protector does not cause any slow paths to be taken when --harmony-species is off by refraining from writing to and reading from the protector when the flag is off. BUG= chromium:606207 R=adamk LOG=Y Review URL: https://codereview.chromium.org/1918383002 Cr-Commit-Position: refs/heads/master@{#35800} [modify] https://crrev.com/b1ec4cb67b7cc41be487b6780fb5c5058901ff9a/src/isolate.cc
,
Apr 26 2016
I'd like to merge https://codereview.chromium.org/1918383002 into 50 and 51. It'll only matter for 51 if you disable Experimental JavaScript features, which would be a temporary workaround for the regression, but it would fix the regression in 50. This patch is very simple, and we probably won't learn much from Canary about it, so I'd be OK with it merging sooner than a Canary run.
,
Apr 26 2016
Let's wait for proper Canary coverage nevertheless. No stable push is planned this week, so there is no rush.
,
Apr 26 2016
Are you able to share the plan for when the next stable push will be?
,
Apr 27 2016
We may have M50 Stable release this week on Thursday and we're planning to cut M50 Stable RC tomorrow (Wednesday) @ 1:00 PM PST.
,
Apr 27 2016
I don't think this is a release blocker. It is an important performance fix but not in the same ballpark as a security or stability fix. It is ok if it skips one stable push. Dan, please merge it back after it has Canary coverage. If everything goes smooth the commit should be on today's Canary. This means you should have enough data in PST morning already to judge if it results in decreased stability. If everything looks fine, please merge.
,
Apr 27 2016
OK,sounds good. Thank you hablich@.
,
Apr 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/37b3223c22878dc3fd7cb5c64db0a9731b136b4d commit 37b3223c22878dc3fd7cb5c64db0a9731b136b4d Author: Dan Ehrenberg <littledan@chromium.org> Date: Wed Apr 27 17:59:58 2016 Version 5.0.71.39 (cherry-pick) Merged b1ec4cb67b7cc41be487b6780fb5c5058901ff9a Do not penalize performance when --harmony-species is off BUG= chromium:606207 LOG=N R=adamk@chromium.org Review URL: https://codereview.chromium.org/1930523003 . Cr-Commit-Position: refs/branch-heads/5.0@{#46} Cr-Branched-From: ad16e6c2cbd2c6b0f2e8ff944ac245561c682ac2-refs/heads/5.0.71@{#1} Cr-Branched-From: bd9df50d75125ee2ad37b3d92c8f50f0a8b5f030-refs/heads/master@{#34215} [modify] https://crrev.com/37b3223c22878dc3fd7cb5c64db0a9731b136b4d/include/v8-version.h [modify] https://crrev.com/37b3223c22878dc3fd7cb5c64db0a9731b136b4d/src/isolate.cc
,
Apr 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/c8a72e53ec2a278e08f4dd690438674d62ea5c8c commit c8a72e53ec2a278e08f4dd690438674d62ea5c8c Author: Dan Ehrenberg <littledan@chromium.org> Date: Wed Apr 27 18:37:12 2016 Version 5.1.281.20 (cherry-pick) Merged b1ec4cb67b7cc41be487b6780fb5c5058901ff9a Do not penalize performance when --harmony-species is off BUG= chromium:606207 LOG=N R=adamk@chromium.org Review URL: https://codereview.chromium.org/1926813003 . Cr-Commit-Position: refs/branch-heads/5.1@{#24} Cr-Branched-From: 167dc63b4c9a1d0f0fe1b19af93644ac9a561e83-refs/heads/5.1.281@{#1} Cr-Branched-From: 03953f52bd4a184983a551927c406be6489ef89b-refs/heads/master@{#35282} [modify] https://crrev.com/c8a72e53ec2a278e08f4dd690438674d62ea5c8c/include/v8-version.h [modify] https://crrev.com/c8a72e53ec2a278e08f4dd690438674d62ea5c8c/src/isolate.cc
,
Apr 27 2016
clean-up approval labels given already merged.
,
Apr 28 2016
,
Apr 28 2016
Although this is fixed for 50, a fix is still needed for 51 and later.
,
May 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/04c8c11ee569a41d4b07839154eb0c718ff6e381 commit 04c8c11ee569a41d4b07839154eb0c718ff6e381 Author: littledan <littledan@chromium.org> Date: Wed May 04 16:47:34 2016 Make array __proto__ manipulations not disturb the species protector Previously, the species protector was invalidated whenever the __proto__ of an Array instance was manipulated. Then, if the map's new_target_is_base field remained set, it was correct to conclude that GetPrototypeOf(array) was %ArrayPrototype%. However, this choice caused the popular D3 framework to invalidate the species protector, causing many functions to become slower. This patch eliminates that aspect of the species protector. Instead, the check is to look at the instance->map()->prototype(). It is valid to look directly at the map's prototype slot, ignoring hidden prototypes and proxies, because - This is only called on Array instances, so the receiver cannot be a Proxy. - For hidden prototypes, any inaccuracy would only result in conservatively taking the slow path. Theoretically, this patch could make methods applied to arrays from other contexts slower. However, the slowdown would only affect a particular array instance and not have a global spill-over effect. Further, the slowdown could be addressed by tracking, either in the instance's map or in the actual prototype object, whether it is a %ArrayPrototype% from any context, in a way which is cheap to query, and use that rather than comparing to the currently executing native context. In interactive testing, this patch led the OnShape CAD system to experience faster load times (110+s -> 40s). BUG= chromium:606207 LOG=Y Review-Url: https://codereview.chromium.org/1936393002 Cr-Commit-Position: refs/heads/master@{#36033} [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/src/builtins.cc [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/src/isolate.cc [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/src/objects-inl.h [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/src/objects.cc [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/src/objects.h [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/src/runtime/runtime-test.cc [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/src/runtime/runtime.h [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/test/mjsunit/harmony/array-species-constructor-accessor.js [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/test/mjsunit/harmony/array-species-constructor-delete.js [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/test/mjsunit/harmony/array-species-constructor.js [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/test/mjsunit/harmony/array-species-delete.js [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/test/mjsunit/harmony/array-species-modified.js [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/test/mjsunit/harmony/array-species-parent-constructor.js [modify] https://crrev.com/04c8c11ee569a41d4b07839154eb0c718ff6e381/test/mjsunit/harmony/array-species-proto.js
,
May 4 2016
Looks like we'll want to let this bake on canary for a couple of days and then merge to 5.1?
,
May 5 2016
,
May 5 2016
Your change meets the bar and is auto-approved for M51 (branch: 2704)
,
May 6 2016
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 6 2016
Please merge your change to M51 branch 2704 by 5:00 PM PST Monday(05/09) so we can take it for next week beta release.Thank you.
,
May 6 2016
I'm currently looking into another performance issue (chromium:609739). I'd like to wait to merge this backport until the fix for that issue can come along with it.
,
May 10 2016
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 10 2016
The follow-on patch in 609739 has been submitted but will take another day to get feedback; then a merge of both will be appropriate assuming all goes well.
,
May 10 2016
Can issue 609739 be made public?
,
May 11 2016
Please merge your change to M51 branch 2704 ASAP (before 5:00 PM PST today). So we can take it for this week beta release tomorrow. Thank you.
,
May 11 2016
The change for https://bugs.chromium.org/p/chromium/issues/detail?id=609739 has just landed in V8 and should get a Canary run tomorrow. When that happens, I will merge the two patches.
,
May 12 2016
OK, if merge happens before Monday 5:00 PM PST or sooner, then we can take it for next week Beta release. Thank you.
,
May 13 2016
Issue 608444 has been merged into this issue.
,
May 13 2016
Please merge your change to M51 branch 2704 before 5:00 PM PST Monday (05/16).So we can take it for next week LAST M51 beta release. Thank you.
,
May 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/b4d8d3e360efefc541f406c1cba0499396b2f55c commit b4d8d3e360efefc541f406c1cba0499396b2f55c Author: Dan Ehrenberg <littledan@chromium.org> Date: Fri May 13 18:18:48 2016 Version 5.1.281.35 (cherry-pick) Merged 04c8c11ee569a41d4b07839154eb0c718ff6e381 Make array __proto__ manipulations not disturb the species protector BUG= chromium:606207 LOG=N R=adamk@chromium.org, adamk Review URL: https://codereview.chromium.org/1979473003 . Cr-Commit-Position: refs/branch-heads/5.1@{#42} Cr-Branched-From: 167dc63b4c9a1d0f0fe1b19af93644ac9a561e83-refs/heads/5.1.281@{#1} Cr-Branched-From: 03953f52bd4a184983a551927c406be6489ef89b-refs/heads/master@{#35282} [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/src/builtins.cc [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/src/isolate.cc [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/src/objects-inl.h [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/src/objects.cc [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/src/objects.h [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/src/runtime/runtime-test.cc [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/src/runtime/runtime.h [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/test/mjsunit/harmony/array-species-constructor-accessor.js [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/test/mjsunit/harmony/array-species-constructor-delete.js [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/test/mjsunit/harmony/array-species-constructor.js [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/test/mjsunit/harmony/array-species-delete.js [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/test/mjsunit/harmony/array-species-modified.js [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/test/mjsunit/harmony/array-species-parent-constructor.js [modify] https://crrev.com/b4d8d3e360efefc541f406c1cba0499396b2f55c/test/mjsunit/harmony/array-species-proto.js
,
May 13 2016
Per comment #42, the change is already merged to M51. Is there anything remaining for M51? If not, please remove "Merge-Approved-51". Thank you.
,
May 13 2016
There were multiple patches merged for 51, so it had both labels at once. The merges are all complete. |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by tkent@chromium.org
, Apr 25 2016