Closed Bug 801488 Opened 12 years ago Closed 7 years ago

Resizing the social sidebar with Facebook loaded inside is janky

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

(Whiteboard: [snappy][leave open][platform-rel-Facebook])

Attachments

(2 files)

don't flush in splitter drags
1.02 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 2: don't call Redraw from nsStackLayout
1.83 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review 
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
That's all GC stuff.
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.)
Yeah, I should be less lazy and get myself set up FB-side so I can try this out...

If you set javascript.mem.options.log, then open the error console with shift-alt-j, it will show you any time there is a GC.
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.
Yeah, comment 0 was from perf.

roc, does the profile in comment 1 mean anything to you?
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.
Group: mozilla-corporation-confidential
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.
Component: SocialAPI → XUL
Product: Firefox → Core
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.
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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-2a22bad28bd8
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).
Depends on: 807934
Whiteboard: [snappy][leave open] → [snappy][leave open][platform-rel-Facebook]
platform-rel: --- → ?
platform-rel: ? → ---
I think this code is dead and/or not used. WONTFIX.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: