545.71 KB, image/png
1.62 KB, patch
Bobby Holley (parental leave - send mail for anything urgent): review+
|Details | Diff | Splinter Review|
FF 47.0 performs poorly when viewing comments on www.washingtonpost.com stories after pausing the comment stream for a few minutes and then resuming the comment stream, if many comments have been added in the interregnum. When this occurs, the "Warning: Unresponsive script" dialog appears, and when you click "Continue", a few comments appear, followed by another "Unresponsive script" dialog, continuing until you run out of patience. When FF is in this state, scrolling the comments is very slow, and memory consumption increases to 1 GB+. It is very difficult to close the offending tab or to close FF when it is in this state; generally you have to kill it from the task manager. To reproduce: 1. Visit http://www.washingtonpost.com; 2. Click on a popular story; 3. Click on the "xxxx comments" box at the story's bottom; 4. Wait for the comments to appear. The story is suitable if new comments appear every few seconds or faster; 5. Pause the comment stream by clicking "Pause live updates"; 6. Wait >= 5 minutes. Spend the time scrolling down and reading existing comments; 7. Scroll back to the top of the comment stream and click "Resume live updates"; 8. Experience the problem. I am using ABP 2.7.3, but no other add-ons. I have had this problem as long as I've been reading washingtonpost.com comment threads, which is at least since FF 33.0 (with correspondingly old ABP versions).
Hi Reporter[q1], I tested this on windows xp with latest nightly and release-47 including the ABP addon. The page is responsive and I am not getting any dialogues. Can you provide "about:support" data as attachment? Thanks
Ack, the problem is back, on FF 48.0 on https://www.washingtonpost.com/news/post-politics/wp/2016/08/17/trump-reshuffles-staff-in-his-own-image/#comments right now. Follow the reproduction directions in comment 0, but leave comments paused for >= 20 min.
Created attachment 8782502 [details] unresponsive.PNG Using the url provided at comment 3, the browser displays "Warning: unresponsive script" dialog when the update is set to live from paused. It takes sometime to reproduce this. First go to the article and pause the live update. After awhile, set the paused update to Live. Then the dialog will appear. The browser becomes very slow after clicking the buttons. --- Version 51.0a1 Build ID 20160817030202 Update Channel nightly User Agent Mozilla/5.0 (Windows NT 5.1; rv:51.0) Gecko/20100101 Firefox/51.0
Can one of you that can reproduce please get a profile? https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Reporting_a_Performance_Problem
(In reply to Andrew Overholt [:overholt] from comment #5) > Can one of you that can reproduce please get a profile? > https://developer.mozilla.org/en-US/docs/Mozilla/Performance/ > Reporting_a_Performance_Problem The browser does not respond to clicking the profiler icon when the problem is occurring. I have obtained a Visual Studio profiler trace while the problem is occurring under FF 48.0.1 instead, which I will answer questions about, but (for privacy reasons) not upload. Here are the top consumers of CPU while the problem is occurring: js::jit::JitcodeGlobalTable::searchInternal 17.16% `js::irregexp::RegExpEmpty::GetInstance'::`2'::dynamic atexit 12.10% destructor for 'instance'' [unknown] 7.30% EnumerateNativeProperties 4.69% ProfileBuffer::addTag 2.64% js::jit::JitcodeGlobalTable::lookupInternal 2.10% SelectorMatches 1.81% js::jit::JitProfilingFrameIterator::moveToNextFrame 1.24% mergeStacksIntoProfile 1.13% JS::ProfilingFrameIterator::getPhysicalFrameAndEntry 1.08% I don't understand why VS cannot determine what "unknown" is. I have loaded all FF symbols and virtually all Windows symbols. In any case, the bug is very easy to reproduce. Just make sure to select an article that's accumulating comments quickly. Leave the comment stream paused for an hour, and you'll probably see the bug every time.
Expanding some of the "unknowns" shows lots of time spent in reflow functions, along with very deep recursion involving such functions as nsBlockFrame::Reflow().
Ah, "[unknown]" is probably the sole module with a VS "failed to load symbols" message: nvwgf2umx.dll, which is part of the NVIDIA video driver stack.
The profiler does not seem to be working in this case. To reproduce the issue, the live comments should be paused for few hours (at least for my case). Screen capture is here: https://testing-1.tinytake.com/sf/OTI2NjAzXzM5MTIyNDc
We're spending tons of time in and under nsDefaultComparator<mozilla::HandleRefPtr<mozilla::StyleSheetHandle>,mozilla::HandleRefPtr<mozilla::StyleSheetHandle>> If we can trust the profile, looks like the Equals is some super slow, doing addrefs and what not. (I have no idea what HandleRefPtr is and why normal RefPtr wouldn't work)
Is this a regression from bug 1244074. I'm pretty sure this is, or at least that added some of the slow paths here.
(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #11) > We're spending tons of time in and under > nsDefaultComparator<mozilla::HandleRefPtr<mozilla::StyleSheetHandle>,mozilla: > :HandleRefPtr<mozilla::StyleSheetHandle>> > If we can trust the profile, looks like the Equals is some super slow, doing > addrefs and what not. Ouch. :( nsDefaultComparator< mozilla::HandleRefPtr<mozilla::StyleSheetHandle>, mozilla::StyleSheetHandle >::Equals(mozilla::HandleRefPtr<mozilla::StyleSheetHandle> const&, mozilla::StyleSheetHandle const&) It's because we're searching for a raw pointer in an array of strong pointers, and HandleRefPtr doesn't have an operator== that works with that. So instead we're ending up constructing a strong reference to do the comparison... > (I have no idea what HandleRefPtr is and why normal RefPtr wouldn't work) The Handle objects are not pointers, and RefPtr only works with pointer typed things.
Created attachment 8784717 [details] [diff] [review] Add operator== overloads for comparing HandlRefPtrs to their raw Handles. Michael, would you mind profiling with this patch applied, to confirm this helps? I tested locally to verify nsTArray<HandleRefPtr<StyleSheetHandle>>::IndexOf(StyleSheetHandle) calls this new operator==.
Comment on attachment 8784717 [details] [diff] [review] Add operator== overloads for comparing HandlRefPtrs to their raw Handles. Review of attachment 8784717 [details] [diff] [review]: ----------------------------------------------------------------- I feel like there's probably some clever template magic we can do here, but these handles aren't long for this earth anyway. Thanks for fixing.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/906c1f635c43 Add operator== overloads for comparing HandlRefPtrs to their raw Handles. r=bholley
Since at least the part which was fixed is a regression, should we take the patch to branches?
I pulled & built central, and have been leaving it sitting on an article for the last little bit. The lag is definitely improved from before, but there is still some scrolling jank. I believe that this jank is present in chrome too, so it might be our fault, but once the cleopatra servers are back up I'll try to post a profile.
Thanks, Michael. Olli, yes I think we should take this on branches.
Comment on attachment 8784717 [details] [diff] [review] Add operator== overloads for comparing HandlRefPtrs to their raw Handles. Approval Request Comment [Feature/regressing bug #]: bug 1244074 [User impact if declined]: performance regression on sites that add many style sheets [Describe test coverage new/current, TreeHerder]: locally tested to verify we no longer unnecessarily AddRef/Release style sheets in this case [Risks and why]: low -- this is just changing the comparison function used by nsTArray searching functions [String/UUID change made/needed]: N/A
[Tracking Requested - why for this release]: performance regression going back to Firefox 47
Comment on attachment 8784717 [details] [diff] [review] Add operator== overloads for comparing HandlRefPtrs to their raw Handles. Fixing a recent(ish) perf regression, Aurora50+
Comment on attachment 8784717 [details] [diff] [review] Add operator== overloads for comparing HandlRefPtrs to their raw Handles. Nice detective work! Let's bring this fix to beta 9. It is late in the cycle but we can try verifying the fix on Friday on beta.
Andrei, can your team test once this lands? Abe, also fine for you to test if you get to it sooner - once it lands on mozilla-beta you can get it from treeherder. Because of the timing of when it lands, I'm not sure who will see it first :)
If it lands in PST working hours, I will get it first. Otherwise Andrei will have it.
We have tested this issue on windows XP with the latest build from treeherder [https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&selectedJob=1566414] In addition to reproducing the existing issue, we also observed other two new issues using this build. Here are our observations: 1.It is still slow and shows the unresponsive script warning dialog (existing issue) 2.Lots of cascades in left and right side of the page (new issue) 3.Unable to resume to live update when “Resume live updates” button is clicked (new issue) Bugs will be filed for these new issues. Here is screen capture: https://testing-1.tinytake.com/sf/OTQ4Nzc3XzM5ODEwMjg --- Version 49.0b9 Build ID 20160831145122 Update Channel beta User Agent Mozilla/5.0 (Windows NT 5.1; rv:49.0) Gecko/20100101 Firefox/49.0
heycam, can you take a look? Should we back this out in 49 since it seems to have caused new regressions? We can try again in 50 to come up with a fix.
Sounds like the regressions in bug 1299908 were caused by work from bug 1296793, not from this perf fix. The slow script issue may be the same as in bug 1284511.
Seems Abe beat me to it. Liz, please ni? me if my team should further look into this. Note that it's more difficult for us to reproduce this bug, because there are close to no new comments displayed while testing washingtonpost.com in our timezone.
The behavior described by this bug still exists in FF 49.0.1. The steps to reproduce are exactly the same. Should I re-open this bug? File another?
(In reply to q1 from comment #34) > The behavior described by this bug still exists in FF 49.0.1. The steps to > reproduce are exactly the same. Should I re-open this bug? File another? At this point, yes. Sorry for the hassle and long lag!
(In reply to Ryan VanderMeulen [:RyanVM] from comment #35) > (In reply to q1 from comment #34) > > The behavior described by this bug still exists in FF 49.0.1. The steps to > > reproduce are exactly the same. Should I re-open this bug? File another? > > At this point, yes. Sorry for the hassle and long lag! OK, I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1313452 awhile back.