Issue metadata
Sign in to add a comment
|
Incrementally constructing DOM containing form controls is slow
Reported by
g...@writerduet.com,
May 25 2016
|
||||||||||||||||||||||||
Issue descriptionChrome Version : 50.0.2631.0 (64-bit) URLs (if applicable) : https://writerduet.com/chrome_slow_dom_test.html Other browsers tested: Add OK or FAIL, along with the version, after other browsers where you have tested this issue: Safari: OK Firefox: OK IE: Chromium right before this version worked great. I binary searched through https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Mac/ and found that is got slow between https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Mac/371206/ and https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Mac/371212/ The difference in performance for me between those two versions is 6 seconds to 13 seconds on the exact same test! What steps will reproduce the problem? (1) https://writerduet.com/chrome_slow_dom_test.html (2) Notice how long it takes on Chrome. Try it on an old version of Chrome (e.g. 49) and it should take about half that! On Firefox it takes *massively* less time for me (1 second) What is the expected result? The alert "should" say about 6 seconds What happens instead? It says about 13 seconds Please provide any additional information below. Attach a screenshot if possible. I suspect the issues is specifically related to appending into a large DOM. This should be very easy to reproduce, and I'm begging you to fix it right away. My application relies on repeated DOM insertions in a a big document, and this regression in Chrome is killing one of my customers (makes a piece of the app DOA with the new Chrome), and likely will be more very soon. I've always told my customers to use Chrome, but this is ruining that. :-(
,
May 26 2016
I just tested on Windows 10 with latest Chrome and recorded on an incognito window of latest Chrome on a Mac - both were 14+ seconds. I just put the older version of Chrome because that's where I first found the issue when I binary-searched all the Chromium builds to see exactly where the regression was introduced. I'm recording the video showing all my settings and the results of two runs on my Mac. Thanks for looking into this! It's definitely a problem. I could see the difference from one auto-build of Chromium to the next.
,
May 26 2016
Thank you for providing more feedback. Adding requester "brajkumar@chromium.org" for another review and adding "Needs-Review" label for tracking. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 26 2016
I verified that it takes 13+ seconds on the same configuration you have - Windows 7, Chrome 51 - using Browserstack. I also tested there using Firefox on Mac, to show you the extreme difference. Video attached.
,
May 30 2016
I bisected this. You are probably looking for a change made after 371206 (known good), but no later than 371212 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/80858159012549fb0d3579d1c77e2ab243ec6ec3..d6193f3c2f6d043b3588290fdbfdefbc88f7a208 This looks a performance regression by Oilpan.
,
Jun 2 2016
This is a major problem for my app. I have a customer reporting basic loading of stuff on my site now takes 4-5 minutes instead of 30 seconds which it did before this. The issue isn't even that I have a ridiculous number of DOM inserts, it's that *any* DOM insert becomes painfully slow once you have a ton of elements. I think you need to revert the commit that said it could be taken out if there were unacceptable stability problems - that is *definitely* the case, and I bet a lot of people are silently suffering because of this. We need an update to Chrome ASAP, please!!
,
Jun 2 2016
,
Jun 2 2016
This seems to be the same problem reported in https://bugs.chromium.org/p/chromium/issues/detail?id=613816. This a major problem for our application as well and we have been getting several customer complaints since Chrome v50 was released. I fully agree that we need a solution ASAP. Please!!!
,
Jun 2 2016
,
Jun 3 2016
If you're going to mark these bugs as duplicates, I need to be able to track the bug you're leading them to, but https://bugs.chromium.org/p/chromium/issues/detail?id=613658 is not viewable. I spent hours of my time tracking down the exact issue, the revision range of the bug being inserted, and giving you a trivial test case to run. This is my second major bug I reported to Chromium, the first was a segfault you guys didn't fix for like a month and just like this one made *my* program look bad even though I wasn't doing anything wrong or unreasonable. This does not encourage me to report issues, or recommend Chrome. I appreciate all you guys do, but I'm not feeling like it's worth me reporting bugs anymore (and I already don't bother you with the minor ones).
,
Jun 3 2016
Sorry about the trouble. We've identified the root cause (it is an editing bug) and discussing a solution in https://bugs.chromium.org/p/chromium/issues/detail?id=613816. I'm checking if I can make the bug public. If I cannot make it public, I'll create a separate bug and copy & paste the information there. I'll make it happen in a couple of hours.
,
Jun 3 2016
Sorry, I meant https://bugs.chromium.org/p/chromium/issues/detail?id=613658.
,
Jun 3 2016
I made https://bugs.chromium.org/p/chromium/issues/detail?id=613658 public. I apologize that the process has made you unhappy. I do want to make Chromium a user-friendly product, so let me know if there's something I can make it better.
,
Jun 3 2016
haraken, are you sure this is a dupe of 613658? How Range objects are created in https://writerduet.com/chrome_slow_dom_test.html ?
,
Jun 3 2016
I appreciate that, though FYI I still can't see the other issue, and I tried two accounts. Check out https://bugs.chromium.org/p/chromium/issues/detail?id=568392 if you want to see my other attempt. I get customer complaints (they think my web app is buggy), spend a lot of time tracking it down to Chrome and giving simple reproduction steps, and then last time it took well over a month to fix. Really glad to hear you guys care about this, though, so I'll continue to report bugs. P.S. I think last time they also duplicated it into a non-public bug, though at that point I had given up on the fix coming in a reasonable time. Why does an open source project have non-public bug reporting, except for security issues that have public vulnerabilities?
,
Jun 3 2016
The reason the bug was non-public is that the bug was reported by the Gmail team and contained a lot of internal profiling data. I removed all of the attachement files and made it public.
,
Jun 3 2016
Re #16: Yeah, I confirmed that the test doesn't create any Range object. This should be a different issue.
,
Jun 3 2016
I don't think this is a duplicate because it doesn't use Range.
I've been trying for a few days now but I haven't been able to reduce the chrome_sow_dom_test.html to a test that doesn't use jquery. JS profile shows Swizzle to be the slow part.
I can create tests that are slightly slower in Oilpan. But not at the scale of chrome_sow_dom_test.html
If you move $("body") outside of the loop, oilpan is just as fast as nonoilpan
,
Jun 8 2016
Just to update, Tested the issue on windows 7 using chrome latest version 53.0.2762.0.The page took almost 11 seconds.Please find the attached screen shot for the same.
,
Jun 8 2016
Speaking for all developers whose product looks terrible because of this (my product was trashed on Reddit because of this issue, where someone reported it was super-laggy on Chrome), we need a fix in the *current* release, not something weeks (or worse) out. This is a major, reputation-ruining regression. I'm at a loss of what to do since I don't know what to work around. This is crushing me... :-(
,
Jun 8 2016
A micro benchmarks (like chrome_slow_dom_test.html) may not represent the real problem in your product. I would like to investigate your product directly so I don't waste time fixing a different problem. Is your product writerduet.com? Could you post the repro steps in your product?
,
Jun 8 2016
Thanks for that consideration! I constructed a fairly intense example for my app to handle, and while it's not blazingly fast on the older Chromiums, I demonstrate in the linked video that the left browser (current Chrome) is noticeably slower than the right browser (Chromium pre the Oilpan), even though I start the left one first. This reproduced for me every time I measure the loading side-by-side. You can test the exact same case here by creating a free account in WriterDuet, using this share link: https://WriterDuet.com/script#DB5ND_JHWH6QHPVKQPB Link to video of the slowness, where the newer browser (left) is outpaced by the older browser (right), even though I give the left one a head start: https://drive.google.com/file/d/0B7XOp5uVvlcTRXUzZWFoVDI2N0E/view?usp=sharing I will also mention that this is on a recent Macbook Pro. The really bad reports are from Windows users, who have said the page consistently locks up on them. These reports are in the minority, so I know this regression affects different situations differently, but I went from ~zero complaints about performance of my app to a bunch very quickly, and more keep coming in.
,
Jun 8 2016
Wow, I just ran this exact same test side-by-side, Chrome vs Safari... and Safari destroyed Chrome. I haven't gone back to try way older Chromes (both my tests before were 50+), but I wonder if Chrome used to get beat like this. I hope this is very easy for you to test and (I pray!) optimize now!! :-) Here's the Chrome vs Safari video: https://drive.google.com/file/d/0B7XOp5uVvlcTQkR1ODZoc19UeDg/view?usp=sharing
,
Jun 8 2016
Wow... just confirmed this used to be MASSIVELY faster in Chrome. I don't have time to binary search which version of Chrome lost the performance edge (it's 5am), but I just tested with Chromium 47 and it's basically the almost as fast as Safari (unlike the current version of Chrome that is literally 4x slower in the last video I posted)! I hope you guys can track down what happened. I suspect the recent Oilpan (or whatever) change just exacerbated a severe regression that was introduced a while back. I'm so happy to find this out, because I'm certain you'll want to resolve it ASAP, and my app's gonna be so much faster once you do! :-D
,
Jun 8 2016
Thanks guy@. There seems to be a much bigger problem than Oilpan. This app creates the DOM incrementally using setTimeout. When the JS adds an <input> checkbox, it triggers the autofill module to update its data, so it scans the entire document for form controls(AutofillAgent::ProcessForms and PasswordAutofillAgent::SendPasswordForms). This results in an O(N^2) type of behavior, causing autofill to use up more than 80% of the time. I'm attaching the trace that shows this. Assigning to vabr@. +tkent do you have any ideas how to avoid this from the blink side? FYI An immediate fix, and a good thing to do in general, is to use something like DocumentFragment to construct your DOM.
,
Jun 8 2016
Thank you!!!! I appreciate the suggestion for a work-around, but I think it would be super-hard for me to switch to DocumentFragment for every insertion (I have a *lot* of DOM manipulations, you're just seeing one case here). I don't think I'm adding (m)any input elements at the stage that's locking up - I do add those elsewhere in initial load and elsewhere, but where it's really slow is after that point when it's mostly dealing with text/spans/divs/classes. Is there any way for my app to turn off the auto-fill behavior in its entirety on the page? And do you think it I literally add autocomplete="off" to all input elements that would solve the problem we're seeing here? P.S. - there still is an issue with Oilpan too, which we shouldn't forget about, because I think that's also causing my (and other people's) users some issues. My original test case might deserve a spin-off, now that we're chasing a bigger fish here?
,
Jun 8 2016
Keishi, is the autofill code kicked by ChromeClient::didAssociateFormControls notification?
If so, I think we can reduce the number of notifications significantly. e.g.
* Document::didAssociateFormControl()
Current code:
if (!m_didAssociateFormControlsTimer.isActive())
m_didAssociateFormControlsTimer.startOneShot(0, BLINK_FROM_HERE);
Change it like:
if (m_didAssociateFormControlsTimer.isActive())
m_didAssociateFormControlsTimer.stop();
m_didAssociateFormControlsTimer.startOneShot(0.3 /* more delay */, BLINK_FROM_HERE);
,
Jun 9 2016
@tkent Yes. That worked. I created a CL . @guy Oilpan is a complete overhaul of the foundational memory management in Blink. Hence it has completely new performance characteristics, i.e. some things are faster, some things are slower. You can write microbenchmarks that test the slow parts, but improving that doesn't necessary help real web sites. chrome_slow_dom_test.html is slow because of getElementsByTagName. This tight loop with dom modification is getElementsByTagName's worst nightmare, because the cache is useless and it needs to scan the document. I don't like that it is 50% slower but if it doesn't effect real web sites, we're fine with it. I investigated the "loading revision changes" phase of you website, and found out getElementsByTagName is not called and querySelectorAll is consuming around 1% of the JS time, so it doesn't explain the slowness (around 30% slow after the autofill fix). I won't fix the benchmark for now but I will try to fix your website! :-)
,
Jun 9 2016
Thanks, Blink team, for the deep analysis and fixes. I'm wondering whether AutofillAgent::didAssociateFormControls might be iterating over some frames multiple times. We could also try to improve PasswordAutofillAgent to take the forms found by AutofillAgent instead of scanning for them again, but that will not decrease the asymptotical complexity. We need to keep observing new forms being added. Marking forms autocomplete=off does not help, we need to scan for the forms to see their attributes. The only way to stop autofill from doing this is to disable it in settings (not possible through page JavaScript). @keishi -- with regards to your last sentence of #29: are you fixing this issue? If there are any concrete suggestions about what we should try to improve in autofill, please let me know.
,
Jun 9 2016
Adding a delay worked well so I haven't looked closely into autofill. https://codereview.chromium.org/2055693002/ But I have a feeling they could be optimized as well to handle large forms.
,
Jun 9 2016
It is possible that AutofillAgent and sibling classes could be optimised. The trouble is that finding those optimisations and carefully testing them and checking against regressions takes time and effort. The password manager team is busy with higher-priority work. This issue is not a regression in the autofill code, and we cannot spend too much resources on it currently.
,
Jun 13 2016
Just to update: Still able to reproduce the issue on Windows 7, Mac 10.11.5, Ubuntu 14.04 using latest M-52(52.0.2743.39). Please find attached screenshot.
,
Jun 15 2016
Please have a fix ready and merge it to M52 branch once it is baked in Canary before 6/22 so that it can be picked up for next beta promotion. Early Stable promotion is scheduled in 2nd week of July. Please plan accordingly.
,
Jun 16 2016
By the way, I appreciate all the insight, but I'm not sure this is the only issue. And I'm scared about it not being available for a month. I have been getting more and more reports of freezing/crashing on my site, and had approximately 0 before right around when I reported the initial bug. The auto-fill problem certainly seems like it caused some huge lag, but what's scarier is I think a more recent change is contributing to the new reports. Since it's very hard to reproduce the crashes (I haven't hit one myself, just lag), pretty much the only way I'll be able to know if you've actually fixed the issue is by getting all the users who've reported problems to try a patched Chrome. And these are mostly not very technical people - they're writers. So if the patch is safe, what I'm begging you to do is merge it into an official Chrome 51 release. I believe irreparable harm is being done to my product's reputation right now (one writer reported the crashes, and told me several people in her writing group have been discussing the same - this likely means tons of my users just think my product is junk), and if this patch won't be widely available for a month or more, it's time for me to tell users to pull the plug on Chrome. That would be very sad, because I've recommended Chrome for years and have always trusted it in the past to have reliable and superior or at least comparable performance to other browsers.
,
Jun 16 2016
A friendly reminder that M52 Stable is launching soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch by July 12. All changes MUST be merged into the release branch by 5pm on July 15 to make into the desktop Stable final build cut. Thank you!
,
Jun 20 2016
Still able to reproduce the issue on Windows 7, Mac 10.11.5, Ubuntu 14.04 using latest M-53(53.0.2773.0) as per steps in comment #0.The alert says about 10.11 seconds. keishi@Could you please provide an update on this issue.
,
Jun 20 2016
I think there's some confusion on the testing methodology, because I originally filed with a very simple test case to show one problem, but we later discovered the *real* problem that was affecting my app was something else, which this bug shifted to fix. The actual testing procedures for the bug trying to be patched here are laid out in my comment 23: https://bugs.chromium.org/p/chromium/issues/detail?id=614856#c23 It's still simple to test, you just need to make a free account in my screenwriting web app, and load the sample script I linked to. I included video links showing the behavior in the current Chrome. I just tested myself using the Mac Chromium available at https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Mac/400636/ (Chromium 53) and observed the same terribly slow behavior as before. Summary: the test you tried is old, but your results are correct - the problem does not appear to have been fixed in the latest build I can find.
,
Jun 22 2016
,
Jul 1 2016
Can someone please assure me this is actually getting fixed? Is there a built executable somewhere I can test on? This massive regression is still really bad for my customers, and I'm very concerned it will not be fully fixed even in the next release.
,
Jul 6 2016
I have not heard anything now in weeks, and have not been able to confirm you've actually fixed this massive problem. Please, don't leave developers hanging like this. I'm extremely concerned that you have not resolved all the issues, and the performance of Chrome in some cases is now terrible, which I don't think you want. Please let me know what's going on, and keep me in the loop to make sure this is 100% resolved before the next release. Thank you very much!
,
Jul 6 2016
Removing myself. Other devs feel free to re-add me if there is anything concrete needed from me.
,
Jul 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6e1c00f9b9656319829c290a280b5ffb07bc4d1a commit 6e1c00f9b9656319829c290a280b5ffb07bc4d1a Author: keishi <keishi@chromium.org> Date: Thu Jul 07 05:20:44 2016 Add delay to didAssociateFormControlsTimer so it doesn't fire too frequently When JS iteratively constructs a DOM tree containing form controls, didAssociateFormControlsTimer was fireing for every iteration, causing a massive slow down. BUG= 614856 Review-Url: https://codereview.chromium.org/2055693002 Cr-Commit-Position: refs/heads/master@{#404083} [modify] https://crrev.com/6e1c00f9b9656319829c290a280b5ffb07bc4d1a/chrome/renderer/autofill/autofill_renderer_browsertest.cc [modify] https://crrev.com/6e1c00f9b9656319829c290a280b5ffb07bc4d1a/chrome/renderer/autofill/password_generation_agent_browsertest.cc [modify] https://crrev.com/6e1c00f9b9656319829c290a280b5ffb07bc4d1a/chrome/test/base/chrome_render_view_test.cc [modify] https://crrev.com/6e1c00f9b9656319829c290a280b5ffb07bc4d1a/chrome/test/base/chrome_render_view_test.h [modify] https://crrev.com/6e1c00f9b9656319829c290a280b5ffb07bc4d1a/components/autofill/content/renderer/autofill_agent.h [modify] https://crrev.com/6e1c00f9b9656319829c290a280b5ffb07bc4d1a/third_party/WebKit/Source/core/dom/Document.cpp
,
Jul 8 2016
,
Jul 8 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jul 8 2016
Your change meets the bar and is auto-approved for M53 (branch: 2785)
,
Jul 8 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jul 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f84b556ff7afe8416c10b9e94d09b4a2a9384f25 commit f84b556ff7afe8416c10b9e94d09b4a2a9384f25 Author: Keishi Hattori <keishi@chromium.org> Date: Fri Jul 08 09:35:49 2016 Add delay to didAssociateFormControlsTimer so it doesn't fire too frequently When JS iteratively constructs a DOM tree containing form controls, didAssociateFormControlsTimer was fireing for every iteration, causing a massive slow down. BUG= 614856 Review-Url: https://codereview.chromium.org/2055693002 Cr-Commit-Position: refs/heads/master@{#404083} (cherry picked from commit 6e1c00f9b9656319829c290a280b5ffb07bc4d1a) Review URL: https://codereview.chromium.org/2129143002 . Cr-Commit-Position: refs/branch-heads/2785@{#56} Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382} [modify] https://crrev.com/f84b556ff7afe8416c10b9e94d09b4a2a9384f25/chrome/renderer/autofill/autofill_renderer_browsertest.cc [modify] https://crrev.com/f84b556ff7afe8416c10b9e94d09b4a2a9384f25/chrome/renderer/autofill/password_generation_agent_browsertest.cc [modify] https://crrev.com/f84b556ff7afe8416c10b9e94d09b4a2a9384f25/chrome/test/base/chrome_render_view_test.cc [modify] https://crrev.com/f84b556ff7afe8416c10b9e94d09b4a2a9384f25/chrome/test/base/chrome_render_view_test.h [modify] https://crrev.com/f84b556ff7afe8416c10b9e94d09b4a2a9384f25/components/autofill/content/renderer/autofill_agent.h [modify] https://crrev.com/f84b556ff7afe8416c10b9e94d09b4a2a9384f25/third_party/WebKit/Source/core/dom/Document.cpp
,
Jul 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e0058d0fbb5c813090e00fdea134c6ba7c493a9e commit e0058d0fbb5c813090e00fdea134c6ba7c493a9e Author: Keishi Hattori <keishi@chromium.org> Date: Fri Jul 08 10:04:52 2016 Add delay to didAssociateFormControlsTimer so it doesn't fire too frequently When JS iteratively constructs a DOM tree containing form controls, didAssociateFormControlsTimer was fireing for every iteration, causing a massive slow down. BUG= 614856 Review-Url: https://codereview.chromium.org/2055693002 Cr-Commit-Position: refs/heads/master@{#404083} (cherry picked from commit 6e1c00f9b9656319829c290a280b5ffb07bc4d1a) Review URL: https://codereview.chromium.org/2134653002 . Cr-Commit-Position: refs/branch-heads/2743@{#598} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/e0058d0fbb5c813090e00fdea134c6ba7c493a9e/chrome/renderer/autofill/autofill_renderer_browsertest.cc [modify] https://crrev.com/e0058d0fbb5c813090e00fdea134c6ba7c493a9e/chrome/renderer/autofill/password_generation_agent_browsertest.cc [modify] https://crrev.com/e0058d0fbb5c813090e00fdea134c6ba7c493a9e/chrome/test/base/chrome_render_view_test.cc [modify] https://crrev.com/e0058d0fbb5c813090e00fdea134c6ba7c493a9e/chrome/test/base/chrome_render_view_test.h [modify] https://crrev.com/e0058d0fbb5c813090e00fdea134c6ba7c493a9e/components/autofill/content/renderer/autofill_agent.h [modify] https://crrev.com/e0058d0fbb5c813090e00fdea134c6ba7c493a9e/third_party/WebKit/Source/core/dom/Document.cpp
,
Jul 12 2016
Tested the issue on Windows 7, Mac 10.11.5, Ubuntu 14.04 using 53.0.2785.14. Observed that the alert says about 9.717 in windows, 9.531 in linux, 10.519 in Mac. Please find attached screenshot. keishi@Could you please confirm the behavior to verify the fix from test team end.
,
Jul 13 2016
Tested the issue on windows 7, Linux Ubuntu 14.04 and Mac 10.11.5 using chrome version 52.0.2743.75 with the below steps 1. Go to URL https://writerduet.com/chrome_slow_dom_test.html 2.Observed the time delay as below Windows :: 10.118 Linux :: 10.30 Mac :: 25.06 Please find the attached screen shot of windows behaviour. keishi@ Could you please confirm on the fix to verify from our end. Thanks,
,
Jul 19 2016
@keishi, could you please update ..
,
Jul 19 2016
I have tested this Chrome version 53.0.2785.8 vs 53.0.2785.21 on Windows, mac and Linux Windows : 11.094 vs 10.718 Mac : 27.736 vs 27.642(safari :25.54) Linux : 10.766 vs 10.839 Note : Not sure if the above results are acceptable numbers or not, So keishi@ or someone on the thread need to confirm the fix is working or not. since I don't see any major difference.
,
Jul 19 2016
I am changing the status back to Assigned since we aren't sure the fix is correct or not, keishi@ please feel free to remark the bug as fixed if the above results were the expected results.
,
Jul 20 2016
On my machine, loading https://WriterDuet.com/script#DB5ND_JHWH6QHPVKQPB is now faster in Chrome than in Firefox 47 so I think this is fixed. Comparison video: https://drive.google.com/file/d/0BwRi59l_ri74cnFJd1RRYmZNQXc/view?usp=sharing |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by brajkumar@chromium.org
, May 26 2016Labels: Needs-Feedback
354 KB
354 KB Download