1175940 - TabChild::RecvUIResolutionChanged can be really slow when switching tabs

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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

Comment 1

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 2

[Bobby Holley (:bholley)]

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?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 5

[Bobby Holley (:bholley)]

(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.

Comment 6

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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

Comment 8

[Gabor Krizsanits (INACTIVE)]

Attachment: WIP

This is just a WIP but I think it helped... I will need to test it some more and maybe improve further.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 10

[Mike Conley (: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!

Comment 11

[Gabor Krizsanits (INACTIVE)]

(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

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: UIResolutionChanged should not trigger sync messages from content to chrome. v1

Try run looks green enough (just a bunch of known intermittent...) so uploading patch for review.

Comment 13

[Mike Conley (: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.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3739fddf04

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7f3739fddf04