1447326 - Tab warming Telemetry on warming state of tabs seems to be pretty wrong

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

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

Somewhere in late February, FX_TAB_SWITCH_REQUEST_TAB_WARMING_STATE probe started providing data that seems pretty wrong:

https://mzl.la/2DHxFGu

In particular, there was a _huge_ jump in the disqualified bucket, and the "loaded" bucket basically hit the floor.

I highly suspect that this probe is broken. We should investigate and fix this.

Comment 1

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

Attachment: Bug 1447326 - Fix tab warming state probe.

Since bug 1432509, the warmingTabs WeakSet has had warmed tabs
cleared from it once they have entered STATE_LOADED. This allowed
us to make decisions on tab switch operations based on whether or
not a tab was still being warmed.

This broke our Telemetry, which assumed that warming tabs would
still be in the warmingTabs WeakSet until either requested or
evicted. Now, instead, we look to see whether or not tab warming
is enabled, and whether tabs _could_ have been warmed in order to
add entries to the right buckets.

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

Comment 2

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

So the problem here was introduced in bug 1432509. What happened was that the warmingTabs WeakSet was originally used as a holding area for tabs that had been warmed but not requested yet - and they were removed from that set when requested OR when evicted after the unload timeout.

The FX_TAB_SWITCH_REQUEST_TAB_WARMING_STATE Telemetry probe depended on that, so that at requestTab time, before removing the warmed tab from the WeakSet, it could check the tabs state and then determine whether or not the warmed tab was fully loaded or not (or had even been warmed in the first place).

Bug 1432509 changed all of that by making the warmingTabs WeakSet hold warmed tabs until their layers are ready, after which, they are no longer warming, but warmed. So that meant that at requestTab time, if the tab was in STATE_LOADED because it had fully warmed, requestTab would think that the tab had never been warmed in the first place, and also isn't in a fit state to be warmed (since it's already loaded), so it'd fallback into the disqualified bucket, which is just nonsense.

So I've broken canWarmTab into canWarmTab and shouldWarmTab. canWarmTab returns true if the tab could potentially be warmed, and shouldWarmTab is used to determine whether or not it makes sense to actually warm the tab. It's maybe a subtle distinction, but it means that we can use canWarmTab when determining in requestTab which buckets to put the requested tab - if it can be warmed, and is in STATE_LOADED, then chances are it was warmed. If it can be warmed, and it's in STATE_LOADING, changes are it was warming but didn't complete. etc.

It's not perfect. This will catch tabs that never had a chance to be warmed - for example, lazily inserted browsers after a session restore. I also considered having another WeakSet, called warmedTabs, where after warming tabs entered STATE_LOADED, they'd get moved to that WeakSet, and we could use that instead for Telemetry. It just seemed like too much complication, but I'm open to revisiting the idea.

What do you think, Doug?

Comment 3

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

Comment on attachment 8961053 [details]
Bug 1447326 - Fix tab warming state probe.

https://reviewboard.mozilla.org/r/229804/#review235588

::: browser/modules/AsyncTabSwitcher.jsm:833
(Diff revision 1)
>      if (!tab) {
>        return false;
>      }
>  
>      // If the tab is not yet inserted, closing, not remote,
>      // crashed, already visible, or already requested, warming

Does "or already requested" belong here anymore?

Comment 4

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

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #2)
> It's not perfect. This will catch tabs that never had a chance to be warmed
> - for example, lazily inserted browsers after a session restore. I also
> considered having another WeakSet, called warmedTabs, where after warming
> tabs entered STATE_LOADED, they'd get moved to that WeakSet, and we could
> use that instead for Telemetry. It just seemed like too much complication,
> but I'm open to revisiting the idea.

I think it's reasonable. It's kind of hard to say where lazily inserted browsers should go anyway, and I don't suspect they'll be overwhelming our numbers. I suppose where they should go depends on what question we want to answer with the "disqualified" and "notWarmed" buckets, which I'm not altogether sure of. I suppose lazily inserted browsers won't result in tab spinners, which means this hurts our ability to assess how well tab warming is preventing spinners? But we can answer questions about that through other means.

I don't know, I'm on the fence. I'm totally fine with this approach if you are.

Comment 5

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

Comment on attachment 8961053 [details]
Bug 1447326 - Fix tab warming state probe.

https://reviewboard.mozilla.org/r/229804/#review235588

> Does "or already requested" belong here anymore?

It doesn't! Good catch.

Comment 6

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

(In reply to Doug Thayer [:dthayer] from comment #4)
> I don't know, I'm on the fence. I'm totally fine with this approach if you
> are.

Cool, thanks for the second opinion. Yeah, okay, let's go this route.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6150424866b1
Fix tab warming state probe. r=dthayer

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/6150424866b1