1408026 - Empty flash when opening new tabs (in particular content process Activity Stream)

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P5)

Description

[Marco Castelluccio [:marco]]

+++ This bug was initially created as a clone of Bug #1383593 +++

With activity stream disabled, new tabs are rendered basically instantly.
With activity stream enabled, rendering is slower.

Screen capture when activity stream is disabled: https://bugzilla.mozilla.org/attachment.cgi?id=8917608.
Screen capture when activity stream is enabled: https://bugzilla.mozilla.org/attachment.cgi?id=8917607.

[Tracking Requested - why for this release]: I would expect tab opening to be one of the most visible things for users.

Comment 1

[Ed Lee :Mardak]

Can you better describe the slowness? From what I see in the video is 1 frame of flashing the background then the Activity Stream content appears. Whereas before with Tiles, there is no flash of just the background.

Comment 2

[Ed Lee :Mardak]

And the flash is only particularly noticeable when you already have about:newtab showing as you can compare that the content briefly disappears then reappears. Whereas if you open a new tab from a different page, the Activity Stream content just appears immediately.

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Ed Lee :Mardak from comment #1)
> Can you better describe the slowness? From what I see in the video is 1
> frame of flashing the background then the Activity Stream content appears.
> Whereas before with Tiles, there is no flash of just the background.

I'm seeing something different, more like what you described in bug 1383593 comment 29.
With activity stream, the tab is opened and you can see the background, then the content appears after ~1 second.
Without activity stream, the tab is opened and you can see the background and the content is already there. The only thing that flashes in this case is the onboarding icon in the top-left corner.

(In reply to Ed Lee :Mardak from comment #2)
> And the flash is only particularly noticeable when you already have
> about:newtab showing as you can compare that the content briefly disappears
> then reappears. Whereas if you open a new tab from a different page, the
> Activity Stream content just appears immediately.

It's actually the same if you open a new tab from e.g. Bugzilla.
With activity stream, the content is shown after ~1 second.
Witout activity stream, the content is shown right away with no flashing.

Comment 4

[Ursula Sarracini (:ursula)]

Attachment: no_flash.mov

The flash is because Activity Stream is running in the content process whereas the old about:newtab is running in the main process. Here's a video of opening new tabs with Activity Stream running in the main process, you can see the content loads immediately, without any flashing

Comment 5

[Marco Castelluccio [:marco]]

(In reply to Ursula Sarracini (:ursula) from comment #4)
> Created attachment 8917867 [details]
> no_flash.mov
> 
> The flash is because Activity Stream is running in the content process
> whereas the old about:newtab is running in the main process. Here's a video
> of opening new tabs with Activity Stream running in the main process, you
> can see the content loads immediately, without any flashing

Yeah, I suspected that was the case.
Is the performance loss from moving in the content process acceptable?

Comment 6

[Ursula Sarracini (:ursula)]

I would say so. We've had loads of discussions with gabor, mconley, and others, about having about:newtab run in the content process, and it's been an open bug for a couple years now. There's a lot more content on the page now coming from many sources (including snippets), so the security of having it run in the content process rather than the main process is definitely desirable and worth the performance difference in this case.

Comment 7

[Ursula Sarracini (:ursula)]

I'm going to post a quick patch to get some feedback about an approach we could take here. Since Activity Stream runs in the content process now, it gets treated like any page that you try to open in a content process - meaning we switch to that tab to show it before waiting for it to be fully loaded. Rather than doing this, we probably want to special case about:newtab to keep showing the visible tab until the requested tab (the new about:newtab you're trying to open) is fully loaded: http://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/browser/base/content/tabbrowser.xml#4544. This gives the illusion of no flash when opening a new tab.

Bug 1389546 attempted to do something to fix this issue by setting the preloaded about:newtab to be active which inadvertently led to that code path getting hit, and making the flash go away. We can sort of use this logic, but instead of messing around with the preloaded browser, we could potentially just force shouldShowBlank to be false if we're requesting about:newtab. Either way, it sounds like the fix lives somewhere in the tab switcher code.

A possible risk with this approach is that we slow down tab switching to be as long as it takes for Activity Stream to load, which could potentially be perceptibly slow. Maybe there's an earlier event than "load" that we could switch tabs for about:newtab that would be late enough to still give the illusion of no flash? Maybe there's an earlier place than updateDisplay() in the tab switcher to put this special case? 

:billm or :mconley, can you shed some light on if this approach is sane?

Comment 8

[Ursula Sarracini (:ursula)]

Attachment: Bug 1408026 - Opening of new tabs is slower if Activity Stream is enabled

Review commit: https://reviewboard.mozilla.org/r/188874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188874/

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Activity stream product team has made a decision to ship Activity streams as is. I'd like to mark this as a fix-optional, if there is a low-risk fix that improves the situation we'd be willing to accept that patch. NI Ed, Jenn to correct me if I am wrong here.

Comment 10

[Ed Lee :Mardak]

(In reply to Marco Castelluccio [:marco] from comment #0)
> Screen capture when activity stream is enabled:
> https://bugzilla.mozilla.org/attachment.cgi?id=8917607.
Looking more closely frame by frame at 60fps, here are the counts from opening a preloaded new tab:
-  5 frames ( 83ms)
- 10 frames (166ms)
- 12 frames (200ms)
-  9 frames (150ms)

So definitely more than my "1 frame" but closer than being 60 frames slow.

Testing on my own machine with Chrome and Nightly:
- Chrome  11 frames
- Chrome  11 frames
- Chrome  12 frames
- Chrome  12 frames
- Chrome  14 frames
- Nightly  4 frames
- Nightly  7 frames
- Nightly  9 frames
- Nightly 10 frames
- Nightly 14 frames

I know "faster than Chrome" isn't the benchmark, so just providing some context. There does seem to be some variance in Nightly… maybe content process starting up?

The patch in attachment 8917984 [details] is touching some relatively complex tabbrowser js code, so I wouldn't feel comfortable uplifting, but maybe others are more comfortable.

Comment 12

[the_goodone1]

(In reply to Marco Castelluccio [:marco] from comment #0)
> +++ This bug was initially created as a clone of Bug #1383593 +++
> 
> With activity stream disabled, new tabs are rendered basically instantly.
> With activity stream enabled, rendering is slower.
> 
> Screen capture when activity stream is disabled:
> https://bugzilla.mozilla.org/attachment.cgi?id=8917608.
> Screen capture when activity stream is enabled:
> https://bugzilla.mozilla.org/attachment.cgi?id=8917607.
> 
> [Tracking Requested - why for this release]: I would expect tab opening to
> be one of the most visible things for users.


Did you experience this bug with Firefox 57 first Betas 3 and 4?

Comment 13

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

(In reply to Ursula Sarracini (:ursula) from comment #7)
> 
> :billm or :mconley, can you shed some light on if this approach is sane?

Hey ursula,

So it sounds like you want to go from this state:

Tab A starts selected
Select Tab B (about:newtab)
<show blank immediately in B>
<eventually render B after X milliseconds>

to

Tab A starts selected
Select Tab B (about:newtab)
<eventually render B after X milliseconds, but continue to show A until this occurs>

is that correct?

Comment 14

[Ed Lee :Mardak]

I think for perceived performance, we do want to "show blank immediately in B". Otherwise, opening new tabs will feel slow.

However, to address this bug, which is most noticeable when opening consecutive new tabs, maybe it could "continue to show A" to avoid the flash of blank… ?

Comment 15

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

Is the concern that the "blank" colour and the background colour of the about:newtab page are not the same?

Comment 16

[Ursula Sarracini (:ursula)]

Mike, that's right I want to continue to show A until B is rendered, but *only* in the case where B is about:newtab, of course.

About the background colour... that's not the concern. The background colour of "blank" actually matches the background colour of Activity Stream so the colour change isn't the issue, it's just the fact that it flashes content at all which looks slow

Comment 17

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

(In reply to Ursula Sarracini (:ursula) from comment #16)
> About the background colour... that's not the concern. The background colour
> of "blank" actually matches the background colour of Activity Stream so the
> colour change isn't the issue, it's just the fact that it flashes content at
> all which looks slow

It sounds like to me, and correct me if I'm wrong here, that what we'd be doing is trading one slowness for another;

We'd trade the perceived tab switch time with the perceived load of about:newtab. So tab switch time would feel slower, but once it's done switching, about:newtab will appear immediately.

To be clear, is that what's being proposed here?

Comment 18

[Ursula Sarracini (:ursula)]

Yeah that sounds right. It's still very much up for discussion what else we could do for perceived performance for loading about:newtab here, but yes we would essentially be trading one slowness for another.

Comment 19

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

(In reply to Ursula Sarracini (:ursula) from comment #18)
> Yeah that sounds right. It's still very much up for discussion what else we
> could do for perceived performance for loading about:newtab here, but yes we
> would essentially be trading one slowness for another.

In that case, I'm not sure that the trade is worth it - or rather, I'm not sure it'd really be improving anything, but just moving pain from one area to another.

I could be wrong though - how long is this amount of time we're talking about? If it's short enough to be mostly imperceivable during the tab switch, but perceivable while showing blank because of the flash, then the trade _might_ be worth it (to go from perceivable to imperceivable).

How long of an interval are we talking about, here? Is it constant?

Comment 20

[Ursula Sarracini (:ursula)]

Ed, do you have any recent numbers on how long it takes for activity stream to render?

Comment 21

[the_goodone1]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #19)
> (In reply to Ursula Sarracini (:ursula) from comment #18)
> > Yeah that sounds right. It's still very much up for discussion what else we
> > could do for perceived performance for loading about:newtab here, but yes we
> > would essentially be trading one slowness for another.
> 
> In that case, I'm not sure that the trade is worth it - or rather, I'm not
> sure it'd really be improving anything, but just moving pain from one area
> to another.
> 

I agree with that, I'm just a normal user but for me tab switch time is more important than about:newtab time. fast tab switching makes Firefox feels snappier.

I know my opinion is not important but I think Opening new tabs performance is good enough now with Firefox 57 beta. If someone needs better performance with Activity Stream, he can disable the highlights from new tab preferences panel and then the new tabs will render instantly with just search bar and top sites.

Comment 22

[Ed Lee :Mardak]

The numbers from comment 10 seem to show on marco's machine, around 150ms to switch in a preloaded about:newtab. If it's not preloaded, marco's video shows it finished rendering at about 1 second. Although the "load" event most likely triggered well before all the top sites rendered.

I'm still actually unclear what this bug is trying to report/address. There seems to be conflicting information from marco's comments stating that with activity stream, content is shown 1 second after opening a new tab, and without activity stream, there's no flashing. Whereas from what I can see from the provided recordings, with activity stream, content is loaded much sooner than 1 second, and without activity stream, there is content appearing and jumping around.

Assuming we're talking about opening a preloaded about:newtab, here's the number of frames (60fps) of blank that I see when opening about:newtab from some other content page:

- 5 frames
- 6 frames
- 14 frames
- 3 frames
- 3 frames
- 5 frames
- 5 frames
- 5 frames
- 4 frames
- 2 frames

And to be clear, the "blank" is when we end up showing the <xul:tabbrowser> where before it was white but then set to match activity stream's background color with https://hg.mozilla.org/mozilla-central/rev/0e14317d2f6c. So for my measurements, I set tabbrowser's color to red, and indeed, the first thing after red is a fully rendered about:newtab.

Comment 23

[Marco Castelluccio [:marco]]

(In reply to Ed Lee :Mardak from comment #22)
> The numbers from comment 10 seem to show on marco's machine, around 150ms to
> switch in a preloaded about:newtab. If it's not preloaded, marco's video
> shows it finished rendering at about 1 second. Although the "load" event
> most likely triggered well before all the top sites rendered.
> 
> I'm still actually unclear what this bug is trying to report/address. There
> seems to be conflicting information from marco's comments stating that with
> activity stream, content is shown 1 second after opening a new tab, and
> without activity stream, there's no flashing.

Ed, the bug is about the flashing that you can see when opening new tabs with Activity Stream, which makes it feel slower.
Compare Ursula's video with my video of Activity Stream. In Ursula's video there's no flashing.

> Whereas from what I can see
> from the provided recordings, with activity stream, content is loaded much
> sooner than 1 second, and without activity stream, there is content
> appearing and jumping around.

You have to ignore the first opened new tab, which is not preloaded. You suggested I file this bug in bug 1383593 comment 29 and from that comment I thought it was pretty clear to you.

Comment 24

[Ed Lee :Mardak]

(In reply to Marco Castelluccio [:marco] from comment #23)
> You have to ignore the first opened new tab, which is not preloaded. You
> suggested I file this bug in bug 1383593 comment 29 and from that comment I
> thought it was pretty clear to you.
Yes, that's why I suggested this bug be split out, but when I asked for clarification, comment 3 makes it sound like it's not about the flashing but for the ~1 second load of activity stream and the non-flashing with activity stream off.

Bug 1379587 is what makes it seem like we've loaded activity stream's page (by using the background color that we will be showing). Bug 1365643 made it so that about:newtab is loaded in the content process, and that allows for the delay resulting in the empty flash.

Comment 25

[Dão Gottwald [:dao]]

I'm not sure what exactly this bug is about (haven't read all 24 comments). Could you please sum this up and explain why this got moved to Tabbed Browser?

Comment 26

[Ed Lee :Mardak]

(In reply to Dão Gottwald [::dao] from comment #25)
> Could you please sum this up and explain why this got moved to Tabbed Browser?
Comment 24 clarified that this bug was split from bug 1383593 to specifically address the slowness (empty flash) when opening a preloaded about:newtab. This bug exists because Activity Stream is in the content process, and we show the Photon background color in between switching tabs.

I believe making Activity Stream run in the main process is not really an option -- it's an option that will be strongly argued against by various people, and Activity Stream doesn't directly control what is shown between switching tabs.

Attachment 8917984 [details] has a potential approach that changes some behavior in tabbrowser.xml to avoid the empty flash.

Comment 27

[Dão Gottwald [:dao]]

<Mardak> dao: i think the resolution for https://bugzilla.mozilla.org/show_bug.cgi?id=1408026 could very well be wontfix as it sounds like the "show empty photon background color" behavior was done for perceived performance

Comment 28

[Florian Quèze [:florian]]

So what's slow here? Is it creating a new content process? loading activity stream? Painting the already-loaded activity stream page in a content process that was busy doing something else? (if the latter, you may want to warm the activity stream tab when hovering the '+' button, see bug 1385453)

Comment 29

[Ed Lee :Mardak]

Here is a profile from 58.0a1 Build ID 20171017141229 with setting to 1 content process. The start time for the 39ms selection is clicking the new tab button (in main process) to getting visibilitychange event (in content process):

https://perfht.ml/2yuTY1r

Comment 30

[Alex Thayer [:alexical] (she/her)]

(In reply to Florian Quèze [:florian] [:flo] from comment #28)
> So what's slow here? Is it creating a new content process? loading activity
> stream? Painting the already-loaded activity stream page in a content
> process that was busy doing something else? (if the latter, you may want to
> warm the activity stream tab when hovering the '+' button, see bug 1385453)

Do we have numbers on how often people use the '+' button vs the keyboard shortcut? I know I pretty much never touch the '+' button, but I don't know how common that is.

I also would have to say that I encourage anyone who is making a decision on this to actually play with FF with the patch applied, because at least to me the difference in perceived performance is surprising, and it _feels_ more like opening a new tab is instant.

Comment 31

[Ed Lee :Mardak]

Attachment: side by side cmd-t-aligned x5

Here's a comparison with the patch. The more relevant case are the right half with preloaded about:newtabs. I believe with the patch, the current tab stays selected longer before switching.

mconley, how should we evaluate this video / proposed fix with the tradeoffs (hopefully recorded)?

Comment 32

[Ed Lee :Mardak]

Attachment: logSwitchTiming slowed 10x

This is just one run with logSwitchTiming, but maybe it shows the tradeoff: we can show a fully rendered about:newtab sooner with an empty flash vs delay showing with no flash. I.e., the delay with no flash can actually increase the actual measured time to show content.

(The initial page is about:blank white, so on the left without the patch, the background changes to the photon color before showing fully rendered page. Whereas on the right with patch, it's white right until it shows the fully rendered page.)

The "change visible" is from the about:newtab when it gets a "visibilitychange" event with `document.visibilityState` == "visible"

Comment 33

[Ed Lee :Mardak]

Attachment: bug1408026.frombugzilla.x5.webmhd.webm

Maybe a step back to better show what bug we're trying to fix. Here's a recording of what users are more likely to experience with the current behavior vs with the potential fix. I.e., open a new tab from some web content page as opposed to from an activity stream page.

The current behavior is that when opening a new tab, we immediately show an empty / blank view for better perceived performance of responding to the user's action.

As noted in the title, it's more noticeable when opening a new tab from activity stream because you can see the content disappear and reappear. But when opening from some web content, there isn't that visual reference.

I'm not sure if we should be optimizing from about:newtab to opening about:newtab at the expense of from web content to opening about:newtab.

Comment 34

[Florian Quèze [:florian]]

(In reply to Ed Lee :Mardak from comment #33)

> I'm not sure if we should be optimizing from about:newtab to opening
> about:newtab at the expense of from web content to opening about:newtab.

I agree with this. Opening several new tabs in a row is unlikely to happen, except maybe when users who care about performance feel that opening a new tab is slow and do it several times to check how it feels...


All of your video recordings seem to be captured on a mac, so I suspect this is a fast developer machine. It may feel differently on a slower machine.

We care about (in priority order) :
- having an immediate reaction to user events (immediate means no more than 100-130ms)
- no flashing (even a very short 16ms flash is potentially felt by users).

So I think if we can display a fully rendered about:newtab within ~100ms, we should avoid the blank paint. If it's going to take longer, removing the previous page from the screen as a first step is good to show we are reacting to the user event.

Would it be possible to delay switching until about:newtab is ready, but wait no more than 100ms? The goal here would be to avoid the flash on fast machines, but stay responsive on all machines.

Comment 35

[Ursula Sarracini (:ursula)]

We're going to wait for bug 1353013 to see if this improves this behaviour

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 58.

Comment 37

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

We now render the layers for Activity Stream when it's in the preloaded state, which means that we shouldn't have to paint a blank frame and wait for Activity Stream to upload layers to the compositor... we should be loading right away.

However, I still see flashes sometimes. I can make this go away if I set the DocShell as active, but I _think_ we want to avoid doing that because preloaded tabs that have active DocShells impact focus in ways that I think we don't want (or at least, need to be sorted out).

Hey k88hudson, do you know if any of the visibility of Activity Stream is kicked off via visibilitychange events? If so, that might account for the remaining flash, since rendering layers doesn't send the visibilitychange event - but activating the DocShell does.

Comment 38

[Kate Hudson :k88hudson]

We do have some code for handling visibilitychange events, but it's mostly used for calculating impression stats/metrics: (e.g. https://github.com/mozilla/activity-stream/blob/7536b339113448ab099b139cc6873076cfe8f1d1/system-addon/content-src/components/Sections/Sections.jsx#L37)

Is there an STR for seeing flash? Is it possible the preloaded page just hasn't loaded yet if you're opening tabs quickly?

Comment 39

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4047

Comment 40

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

Okay, I've dug into this a bit, and here's what I think is happening.

Right now, preloaded browsers are given the instruction to paint and upload their layers to the compositor, but the DocShells are made inactive. That means that things like the visibilitychange event doesn't fire. That RenderLayers happens once, forces the paint, and then no more paints seem to happen in that background tab. I suspect this is because the RefreshDriver isn't ticking for it for some reason (which makes sense - I don't think we want to necessarily tick the refresh driver on the preloaded browser).

So that paint happens, and it can be requested even before the document has finished loading, I believe. This means that a blank frame gets uploaded for the tab, and then no more paints happen until the DocShell is set as active.

At that point, we have a race - the DocShell becomes active and we either paint before the next composite, or we don't. In the former case, there's no blank frame flash, and in the latter, there is.

One thing we could try is to have the parent process only request RenderLayers once Activity Stream has more or less settled. Apparently, the "hydration" message is a good one to listen for. If we do that, and then remove the renderLayers = true inside tabbrowser.js, at least locally for me, this eliminates the flash.

Tentative pull request to Activity Stream is here: https://github.com/mozilla/activity-stream/pull/4047/

Comment 41

[[github robot]]

Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/029216d173836a4efed592dbcf60a828219be5b0
Fix Bug 1408026 - Render layers for the preloaded newtab after hydration.

https://github.com/mozilla/activity-stream/commit/51f2bdcddcd5ec3d4254871e4cb89d8973e12da7
Merge pull request #4047 from mikeconley/hydrate-fix

Fix Bug 1408026 - Render layers for the preloaded newtab after hydration

Comment 42

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

I'm actually going to re-open this - there's a second piece needed once the thing in Activity Stream gets exported to mozilla-central - we need to remove the early call to renderLayers from within tabbrowser.js.

Comment 43

[Ed Lee :Mardak]

mconley, does the tabbrowser change need to land at the same time as the activity stream change? Or earlier/later are okay?

Comment 44

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/acf2e3b5fc6b

Comment 45

[Ed Lee :Mardak]

mconley, activity-stream change is now in m-c with bug 1447797 export. Were you going to do the tabbrowser renderLayers change here?

Comment 46

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

Attachment: Bug 1408026 - Remove early renderLayers for preloaded browsers now that Activity Stream takes care of it.

Review commit: https://reviewboard.mozilla.org/r/231684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231684/

Comment 47

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

See comment 42 and comment 45 on why I'm requesting this partial revert of bug 1443315.

Comment 48

[Florian Quèze [:florian]]

Comment on attachment 8962881 [details]
Bug 1408026 - Remove early renderLayers for preloaded browsers now that Activity Stream takes care of it.

https://reviewboard.mozilla.org/r/231684/#review237254

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0b35d51fb5f
Remove early renderLayers for preloaded browsers now that Activity Stream takes care of it. r=florian

Comment 50

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d0b35d51fb5f