Closed Bug 801488 Opened 7 years ago Closed 3 years ago
Resizing the social sidebar with Facebook loaded inside is janky
STR: In latest nightly with Facebook integration turned on, resize the social sidebar. Results: Sidebar does not resize smoothly; instead, it janks, apparently due to CPU usage. perf shows that we're spending most of our time in JS. > 5.39% firefox libxul.so js::GCMarker::drainMarkStack(js::SliceBudget&) > 2.86% firefox libxul.so js::ion::ToggleBarriers(JSCompartment*, bool) > 2.68% firefox libxul.so js::gc::FinalizeArenas(js::FreeOp*, js::gc::ArenaHeader* > 2.12% firefox libxul.so JSScript::markChildren(JSTracer*) > 2.07% firefox libxul.so js::gc::MarkString(JSTracer*, js::EncapsulatedPtr<JSAtom > 1.73% firefox libxul.so js::gc::ScanShape(js::GCMarker*, js::Shape*)
Summary: Resizing the social sidebar is janky → Resizing the social sidebar with Facebook loaded inside is janky
Here's a Cleopatra profile. http://people.mozilla.com/~bgirard/cleopatra/?report=acdfcf96661b26d1e2274cf103e283b5690ec6dd
That's all GC stuff.
(in comment 0)
You should re-profile yourself to be sure, because I might have just gotten unlucky. (Or just check to see if we're gc'ing like crazy.)
Okay okay. :) I don't think it's GC; my profile must have caught it at a bad time. Maybe the cleopatra profile is better.
Oh, I assumed the numbers were from SPS. Yeah, no GC in that one. Cancel the alert, Bill! Looks like it is mostly layout::Flush and nsRefreshDriver::Notify, which I don't know anything about.
Did you have any chat windows open? The chatbox window-adjusting code is pretty flush-happy. Hopefully that shouldn't be a problem when no chat windows are opened, but maybe we're doing something dumb.
> Did you have any chat windows open? No, just the sidebar.
The profile's a bit hard to interpret with just the labels. Could we get a profile with proper symbols and sampling?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11) > The profile's a bit hard to interpret with just the labels. Could we get a > profile with proper symbols and sampling? Sorry, what would you like changed about the profile from comment 1?
It appears to contain only sample labels, not full C++ stacks.
Which page did you have loaded in the content area when you were testing this? Does the behavior differ depending on the loaded page? I get pretty smooth resizing with just the social sidebar with Facebook open next to e.g. about:config.
With about:blank, resizing the sidebar via the splitter is pretty smooth for me. However, resizing the Firefox window is not. Eg, a "quick" drag of the left app border to make the app larger shows (a) sidebar slides to the left revealing white space (2-5cm, depending on drag speed) at the right border (b) reflow causes sidebar to jump the 2-5cm back to the right. This looks janky! Same basic thing in reverse when dragging so the app shrinks. Justin - is this maybe what you are seeing?
> This looks janky! Same basic thing in reverse when dragging so the app shrinks. > > Justin - is this maybe what you are seeing? Yes, and if I dig down into the Cleopatra stack, I see layout::Flush calling into a resize handler in Facebook code. It looks like that's the problem. > It appears to contain only sample labels, not full C++ stacks. Ah, I see. For some reason Cleopatra doesn't want to turn on stackwalking, even though I'm in a nightly build. I guess it doesn't matter here, since the culprit appears to be in JS. I'll try to figure it out next time I profile something which crosses the C++/JS boundary (since, when it's entirely in C++, perf works great).
With about:blank in the content area and dragging the left-hand app border, the sidebar itself doesn't resize - only the about:blank content area does - so I don't understand why a resize handler in the Facebook sidebar content is being called?
Stackwalking is disabled on Linux in the built-in profiler due to bug 779291, so that explains that.
This is a better profile: http://people.mozilla.com/~bgirard/cleopatra/?report=8d9e73fb219a861c3191483f724642abeaebe932 The GC stuff is a red herring. It just seems like an expensive reflow.
A couple of interesting things in that profile: Most of the reflow flushing is under nsSplitterFrameInner::AdjustChildren calling FlushPendingNotifications(Flush_Display). Let's just stop doing that, it's almost certainly a deoptimization at this point. As far as I can tell there is no need to flush here. If we don't flush, we may be able to coalesce more work. I'll attach a patch for that. 36% of the profile is nsStackLayout::Layout calling Redraw calling nsIFrame::InvalidateFrameSubtree which recursively walks a large frame subtree. We can probably just remove that Redraw call (similar to what we did in bug 750417).
This is a considerably riskier patch. I have not tested it. It's hard to even know how to test it, but as in bug 750417, it *should* work. It would be useful for someone to test this patch with the previous patch to see if it helps this bug.
Assignee: nobody → roc
Attachment #674490 - Flags: review?(matt.woodrow)
Comment on attachment 674487 [details] [diff] [review] don't flush in splitter drags Hmm. Yeah, at this point I bet that's mostly a no-op in terms of actually painting things, eh? r=me
Attachment #674487 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #23) > Hmm. Yeah, at this point I bet that's mostly a no-op in terms of actually > painting things, eh? Yes, that's a very good point.
Attachment #674490 - Flags: review?(matt.woodrow) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22) > Created attachment 674490 [details] [diff] [review] > Part 2: don't call Redraw from nsStackLayout > > It would be useful for someone to test this patch with the previous patch to > see if it helps this bug. TBH I don't see a huge amount of difference with this patch, but resizing the splitter itself never seemed too janky to me, so I'd probably have to measure things to know for sure. Where I did notice jank is when resizing the right-hand app border - I see the same stuttering of the app border when the sidebar is both visible and invisible - but it *looks* worse when it is visible as the entire sidebar stutters along with the border. This patch hasn't addressed that (but IIUC, it isn't expected to have). Sorry, not much useful here, but thought it worth sharing anyway.
Then another profile with these patches applied would be good.
http://people.mozilla.com/~bgirard/cleopatra/?report=93cf95d1272492fa6e6a21443033a6a9112d2ca4 is my first attempt at such a profile, so I'm not sure how useful it actually is
(In reply to Mark Hammond (:markh) from comment #27) > http://people.mozilla.com/~bgirard/cleopatra/ > ?report=93cf95d1272492fa6e6a21443033a6a9112d2ca4 is my first attempt at such > a profile, so I'm not sure how useful it actually is This profile doesn't appear to have symbols (all the function names in xul.dll have hex values instead of names). Maybe you're not building with sufficient debug info? Maybe you can try copying from the nightly mozconfigs in browser/config/mozconfigs?
(In reply to comment #28) > (In reply to Mark Hammond (:markh) from comment #27) > > http://people.mozilla.com/~bgirard/cleopatra/ > > ?report=93cf95d1272492fa6e6a21443033a6a9112d2ca4 is my first attempt at such > > a profile, so I'm not sure how useful it actually is > > This profile doesn't appear to have symbols (all the function names in xul.dll > have hex values instead of names). > > Maybe you're not building with sufficient debug info? Maybe you can try > copying from the nightly mozconfigs in browser/config/mozconfigs? If you do your own builds, you need to add --enable-profiling in your mozconfig.
I just tried with a new build and have the same symbol problem. I have *some* symbols but not all - eg, I see Input::nsInputStreamPump::OnInputStreamReady, js::RunScript, Paint::PresShell::Paint, Timer::Fire, Startup::XRE_Main etc, but nothing for what Roc says is in xul.dll. I certainly have --enable-profiling enabled and copied mozconfig setttings from browser/config/mozconfigs - so I'm at a bit of a loss. It's even possible it's related to bug 796526 - all my builds (and many on tbpl) would spew warnings about SymGetModuleInfo64 failing to get certain symbols. So it seems I can't help get a useful profile here...
> So it seems I can't help get a useful profile here... Worst comes to worst you could push the patch to try and download an opt build...
(In reply to Justin Lebar [:jlebar] from comment #32) > > So it seems I can't help get a useful profile here... > > Worst comes to worst you could push the patch to try and download an opt > build... hrmph - did that, same result. Strange. I guess I'll try a native 64bit build next...
Same result with a 64bit release build. A 64bit debug build hit bug 670915. So I'm officially giving up :)
(In reply to Mark Hammond (:markh) from comment #34) > Same result with a 64bit release build. A 64bit debug build hit bug 670915. > So I'm officially giving up :) If you have it in you, you should speak to BenWa about the issues you've been having. He may be able to be helpful.
If you can post the tryserver link, I can see if the jank is improved, and maybe even get a profile. (I searched through try but didn't find any pushes from your e-mail address in the past few days.)
Oh, it's not @mozilla.com, I see! https://tbpl.mozilla.org/?tree=Try&rev=6baebf3608d8 I am not at all surprised that you're not getting stacks if you're testing on Windows.
Linux and Mac builds coming at https://tbpl.mozilla.org/?tree=Try&rev=2a22bad28bd8
Try run for 2a22bad28bd8 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=2a22bad28bd8 Results (out of 2 total builds): success: 2 Builds (or logs if builds failed) available at: http://email@example.com
Whiteboard: [snappy] → [snappy][leave open]
Either these patches or bug 804323's patch caused mochitest browser-chrome crashes on all platforms. Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/293bdc2afe11 https://tbpl.mozilla.org/php/getParsedLog.php?id=16451037&tree=Mozilla-Inbound 0 libxul.so!nsGenericHTMLElement::GetOffsetRect(nsRect&, nsIContent**) [nsGenericHTMLElement.cpp : 490 + 0x6] eip = 0x013e5761 esp = 0xbfbc2910 ebp = 0xbfbc2998 ebx = 0x027a1234 esi = 0x91bb1858 edi = 0x8b213228 eax = 0xf0dea7ff ecx = 0x027a1234 edx = 0x8b0b7e00 efl = 0x00010296 Found by: given as instruction pointer in context 1 libxul.so!nsGenericHTMLElement::GetOffsetHeight(int*) [nsGenericHTMLElement.cpp : 625 + 0x17] eip = 0x013e3cea esp = 0xbfbc29a0 ebp = 0xbfbc29e8 ebx = 0x8d991da0 esi = 0xbfbc29dc edi = 0x8f3fa420 Found by: call frame info 2 libxul.so!nsIDOMHTMLElement_GetOffsetHeight [dom_quickstubs.cpp : 12640 + 0xb] eip = 0x017c484e esp = 0xbfbc29f0 ebp = 0xbfbc2a38 ebx = 0x8d991da0 esi = 0xb2fff8a0 edi = 0xa63b2024 Found by: call frame info 3 0x3b1d326 eip = 0x03b1d327 esp = 0xbfbc2a40 ebp = 0xbfbc2a88 ebx = 0x8d991da0 esi = 0x8aafe010 edi = 0xb2fff8b0 Found by: call frame info 4 libxul.so + 0x1aea233 eip = 0x027a1234 esp = 0xbfbc2a90 ebp = 0xb2fff848 Found by: previous frame's frame pointer 5 libxul.so!js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) [MethodJIT.cpp : 1043 + 0x1c] eip = 0x022273da esp = 0xbfbc2aa0 ebp = 0xb2fff848 Found by: stack scanning 6 libxul.so!mozilla::css::Loader::InsertSheetInDoc(nsCSSStyleSheet*, nsIContent*, nsIDocument*) [Loader.cpp : 1310 + 0x5] eip = 0x01227079 esp = 0xbfbc2ac0 ebp = 0xb2fff848 Found by: stack scanning 7 libxul.so!js::mjit::JaegerShot(JSContext*, bool) [MethodJIT.cpp : 1101 + 0x8] eip = 0x02227567 esp = 0xbfbc2b10 ebp = 0xb2fff848 Found by: stack scanning 8 libc-2.11.so + 0xdab47 eip = 0x00bc2b48 esp = 0xbfbc2b30 ebp = 0xb2fff848 Found by: stack scanning 9 libxul.so!js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) [jsinterp.cpp : 2420 + 0x15] eip = 0x020f9981 esp = 0xbfbc2b70 ebp = 0xb2fff848 Found by: stack scanning 10 libxul.so!XPCWrappedNative::GetWrappedNativeOfJSObject(JSContext*, JSObject*, JSObject*, JSObject**, XPCWrappedNativeTearOff**) [XPCWrappedNative.cpp : 1869 + 0xa] eip = 0x017b6af7 esp = 0xbfbc2b90 ebp = 0xb2fff848 Found by: stack scanning
FWIW the try build seems to work a lot better for me. But I think the speed of scrolling depends at least somewhat on how many items are in your newsfeed, and I didn't leave the tryserver build up for particularly long.
(In reply to Mark Hammond (:markh) from comment #27) > http://people.mozilla.com/~bgirard/cleopatra/ > ?report=93cf95d1272492fa6e6a21443033a6a9112d2ca4 is my first attempt at such > a profile, so I'm not sure how useful it actually is You're doing a local build on windows so you have to setup a local symbol server since the nightly symbol server doesn't know about your build. The instruction are outlined here: https://developer.mozilla.org/en-US/docs/Performance/Profiling_with_the_Built-in_Profiler#Profiling_local_Windows_builds At one point we will make the add-on set this up for you.
The crash was caused by bug 804323. I will reland these patches.
We need someone (Justin?) to evaluate if this is fixed or not.
I tried earlier today but couldn't because of bug 806267 (resizing is completely broken).
No longer depends on: 807934
Assignee: roc → nobody
4 years ago
Whiteboard: [snappy][leave open] → [snappy][leave open][platform-rel-Facebook]
3 years ago
platform-rel: ? → ---
I think this code is dead and/or not used. WONTFIX.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.