Closed Bug 1175940 Opened 4 years ago Closed 4 years ago

TabChild::RecvUIResolutionChanged can be really slow when switching tabs

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
e10s m8+ ---
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: mconley, Assigned: gkrizsanits)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Here's a profile I just gathered while tab switching on OS X on a standard resolution display while my machine was under heavy load (doing a build in the background).

It looks like when we select a tab, the tab receives the UIResolutionChanged message, and then sends a sync message up to the parent asking for the current DPI.

This seems horrifically inefficient - especially in this profile, where handing UIResolutionChanged takes almost 1 second, in the section that I'm looking.

https://people.mozilla.org/~sguo/cleopatra/#report=dcf25dd16271356a002c70ca7bf0a5f9a7579f6d&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A413117,%22end%22%3A414786%7D%5D&selection=0,5298,5299,5300,5301,4967,5302,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,981,982,983,984,985,986,5316,5409
Assignee: nobody → bobbyholley
This might be related to bug 1180916 - like, this might be a silly sync message that we never need to send, but that the real problem is that we're sending a sync message when the parent is busy painting a frame.

It's possible that eliminating this sync message will just create space for other sync messages to also be blocked by activity in the parent.
Hey Brad,

Bill says you investigated this a little bit. Can you update me on what you found? He suggested that it might be more useful for me to look at bug 1170484 first - would you prefer that?
Flags: needinfo?(blassey.bugs)
Noticed that this behaviour is triggered via swapFrameLoaders - example, opening Gecko Profiler panel causes UI resolution message to be sent to every TabChild in the window.

Sorry for brevity - on phone. Can supply more detail tomorrow.
Flags: needinfo?(mconley)
Assignee: bobbyholley → gkrizsanits
I did poke at this, but couldn't reproduce what mconley was seeing. Based on comment 3, this might be an artifact of the profiler though, which means we probably care a whole lot less.
Flags: needinfo?(blassey.bugs)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> I did poke at this, but couldn't reproduce what mconley was seeing. Based on
> comment 3, this might be an artifact of the profiler though, which means we
> probably care a whole lot less.

Ok, gabor is going to take over here, and should probably put it at the bottom of his list given that.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> Noticed that this behaviour is triggered via swapFrameLoaders - example,
> opening Gecko Profiler panel causes UI resolution message to be sent to
> every TabChild in the window.
> 
> Sorry for brevity - on phone. Can supply more detail tomorrow.

I could not reproduce it either so far. The Gecko Profiler you mentioned is the Add-on or the built in profiler? The later does not seem to work on the nightly for some reason, maybe it's because of the Add-on singing... Anyway, some more detailed step to reproduce this issue would be great when you have some time for it.
Talked to gabor about this in IRC. It looks like we send (at least) one UIResolution message to each TabChild after a swapFrameLoaders[1], which is probably excessive. Each message seems to result in the child synchronously asking the parent for the latest DPI value for the screen each browser is on. Again, seems excessive.

[1] This is called in (at least) the following cases:

a) Tab tear in / tab tear out
b) Opening up an SDK panel
c) Opening customize mode
Flags: needinfo?(mconley)
Attached patch WIP (obsolete) — Splinter Review
This is just a WIP but I think it helped... I will need to test it some more and maybe improve further.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> Created attachment 8643719 [details] [diff] [review]
> WIP
> 
> This is just a WIP but I think it helped... I will need to test it some more
> and maybe improve further.

After reading a bunch of code, I feel like this should really fix the problem. But I would
love to do some more through testing. Mike, could you give me some hints how to make sure
the performance issue is really fixed with the patch? So basically something like what you did in comment 1.
Flags: needinfo?(mconley)
I ran this patch with the profiler for a bit, and looked for sync messages caused by UIResolutionChanged, and I couldn't find any. 

So, to sum, this seems to eliminate the sync messages that UIResolutionChanged was causing. Great work!
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> I ran this patch with the profiler for a bit, and looked for sync messages
> caused by UIResolutionChanged, and I couldn't find any. 

Thanks for your help Mike!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cf001f8b415
Try run looks green enough (just a bunch of known intermittent...) so uploading patch for review.
Attachment #8643719 - Attachment is obsolete: true
Attachment #8646915 - Flags: review?(mconley)
Comment on attachment 8646915 [details] [diff] [review]
UIResolutionChanged should not trigger sync messages from content to chrome. v1

Review of attachment 8646915 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable to me, and I feel reasonably confident reviewing this code.

Thanks so much gabor!

::: dom/ipc/TabChild.cpp
@@ +2897,5 @@
>  {
>    ScreenIntSize oldScreenSize = GetInnerSize();
>    mDPI = 0;
>    mDefaultScale = 0;
> +  static_cast<PuppetWidget*>(mPuppetWidget.get())->UpdateBackingScaleCache(aDpi, aScale);

As this was the only consumer of ClearBackingScaleCache, we can probably remove that method now.
Attachment #8646915 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/7f3739fddf04
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.