1730284 - Loading recent DOOM files on wadcmd.com takes ~40s on Firefox, ~2s in Chrome

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: wadcmd-profile-chrome.json

Basic information

Steps to Reproduce:

• Visit wadcmd.com (it's a website that runs the DOOM game in WASM)
• Note: Doom game assets are stored in ".wad" files. I used the ".wad" file associated with the purchasable game but you can presumably reproduce this with the free shareware files linked from the site
• Click "doom.wad" in Recent files (note: it caches files you've previously opened. For convenience I used this functionality but you might be able to reproduce through the "Open file(s)" link selector instead)

Expected Results:
Firefox and Chrome load times are roughly comparable

Actual Results:
Firefox takes ~40s while Chrome takes ~2s. Curiously, Firefox does not seem to be maxing out the CPU and the file it's probably loading is only a few MiB so it seems weird to take so long.

Note: I've played this for a bit in Chrome but haven't in Firefox – maybe things are better cached?

---

More information

Screenshot: (if relevant, please attach a screenshot or screencast to this bug report)

Profile URL: (follow the instructions at https://profiler.firefox.com/)

• Firefox: https://share.firefox.dev/3A4IGzE
• Chrome (attached)

Basic systems configuration:

OS version: macOS 11.5.2

GPU model: AMD Radeon Pro 5300M 4 GB
Intel UHD Graphics 630 1536 MB

Number of cores: 8

Amount of memory (RAM): 64 GB

Thanks so much for your help.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Clicking Phase 1 for example does show the difference. Not much CPU is used when initializing the game.
I get similar profile locally as in comment 0. Child process is almost empty but it does get vsync.
The issues seems to be about throttling hidden iframes. If I change layout.throttled_frame_rate to 60, loading happens very fast.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

We probably shouldn't throttle hidden iframes if they have explicit rAF callbacks registered.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:emilio, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

I don't know how to reproduce this, but Olli, since you could, can you check that changing layout.oopif_activity_grace_period_ms to something large also fixes it?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

layout.oopif_activity_grace_period_ms doesn't help here. I tried with value 60000.
layout.throttled_frame_rate 60 does help.

Surprising if you can't reproduce. Just clicking "Phase 1" link in the page should do it.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:emilio, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Ah, d'oh, I didn't realize there were premade files, I just saw the big blue button.

So the relevant iframe is styled like this:

#inspect {
  position: fixed;
  top: 0;
  right: 0;
  width: 0;
  height: 0;
  border: none;
}

And is same-origin. Just using width: 1px; height: 1px; or so makes it fast... So we're hitting this code-path: https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/dom/base/Document.cpp#7064

So... This code comes from bug 1145439.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: RAF throttling test-case.

Olli, seems like Chrome doesn't throttle RAF at all for the foreground tab. Thoughts on whether we should just do that?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Not throttling would be at least less surprising for the web devs. I wonder why we started to throttle.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

So, same-origin-test-case would be comment 8 or https://crisal.io/tmp/raf-throttle.html

Cross-origin test-case would be http://crisal.io/tmp/raf-throttle.html

It seems Chrome throttles display: none cross-origin iframes by a factor of three, and completely stops offscreen cross-origin iframes (which doesn't make all that much sense to me?).

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Safari seems to throttle offscreen iframes unconditionally, and Chrome canary does seem to throttle display: none cross-origin iframes. So this is a bit messy

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dholbert, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

I think I have a reasonably good idea to fix this.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1730284 - Factor out some IntersectionObserver code. r=smaug

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1730284 - Use whether the embedder element intersects the viewport to decide whether to throttle in-process iframes. r=smaug

This is more likely to be understandable by developers, matches other
browsers more closely (see bug comments), and seems more in-line with
what we do for OOP iframes.

Add a pref to not do this throttling at all (which would match Chrome),
though this is probably good enough for now.

Depends on D146573

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Florian, do you have any concerns with how changing the throttling behavior could affect power usage?

Emilio, it may help if you could summarize how your change could affect power usage so Florian doesn't need to dig too deep.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

See comment 10+ for an analysis. The tldr is that we now throttle and would stop throttling 0-sized and visibility: hidden / opacity: 0 same-origin iframes. Behavior otherwise should be mostly unchanged, and other browsers don't seem to throttle these.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ec157459e8c
Factor out some IntersectionObserver code. r=smaug,sefeng
https://hg.mozilla.org/integration/autoland/rev/98834b863104
Use whether the embedder element intersects the viewport to decide whether to throttle in-process iframes. r=smaug

Comment 19

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1730284) for causing devtools,web-platform and mochitest failures CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=98834b863104cc6e69214d86aac24179be55bd09&selectedTaskRun=AUCfqcHWQGCdTJkupVn2jw.0

Log: https://treeherder.mozilla.org/logviewer?job_id=379282305&repo=autoland&lineNumber=4367
https://treeherder.mozilla.org/logviewer?job_id=379282683&repo=autoland&lineNumber=5607
https://treeherder.mozilla.org/logviewer?job_id=379284333&repo=autoland&lineNumber=5092

Backout: https://hg.mozilla.org/integration/autoland/rev/b913035865bcceaf618de9b21c66e6b04bd6634e

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/067f25c4e743
Factor out some IntersectionObserver code. r=smaug,sefeng

Comment 21

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/067f25c4e743

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Ok so the devtools tests I got figured out. Boris, it seems we're asserting rather frequently in scroll-animations/css/at-scroll-timeline-dynamic.tentative.html after my change, and I'm pretty sure it's not a bug in my code.

The assert is this:

      MOZ_ASSERT(timeline->IsMonotonicallyIncreasing(),
                 "Don't put non-MonotoniciallyIncreasing timeline into "
                 "PendingAnimationTracker");

We end up there because the test creates a regular CSS animation which is added to the pending animation tracker here:

(rr) bt
#11 mozilla::PendingAnimationTracker::AddPending(mozilla::dom::Animation&, nsTBaseHashSet<nsRefPtrHashKey<mozilla::dom::Animation> >&) (this=0x7f9b1558ee40, aAnimation=..., aSet=...)
    at /home/emilio/src/moz/gecko-6/dom/animation/PendingAnimationTracker.cpp:31
#12 mozilla::PendingAnimationTracker::AddPlayPending(mozilla::dom::Animation&) (this=0x7f9b1558ee40, aAnimation=...)
    at /home/emilio/src/moz/gecko-6/dom/animation/PendingAnimationTracker.h:39
#13 0x00007f9b1f1868ee in mozilla::dom::Animation::PlayNoUpdate(mozilla::ErrorResult&, mozilla::dom::Animation::LimitBehavior)
    (this=this@entry=0x7f9b155e5500, aRv=..., aLimitBehavior=aLimitBehavior@entry=mozilla::dom::Animation::LimitBehavior::Continue) at /home/emilio/src/moz/gecko-6/dom/animation/Animation.cpp:1448
#14 0x00007f9b1f18d515 in mozilla::dom::Animation::Play(mozilla::ErrorResult&, mozilla::dom::Animation::LimitBehavior) (this=0x7f9b155e5500, aRv=..., aLimitBehavior=mozilla::dom::Animation::LimitBehavior::Continue)
    at /home/emilio/src/moz/gecko-6/dom/animation/Animation.cpp:639
#15 mozilla::dom::CSSAnimation::PlayFromStyle() (this=0x7f9b155e5500)
    at /home/emilio/src/moz/gecko-6/dom/animation/CSSAnimation.cpp:104
#16 0x00007f9b2182162e in BuildAnimation(nsPresContext*, mozilla::NonOwningAnimationTarget const&, nsStyleUIReset const&, unsigned int, ServoCSSAnimationBuilder&, mozilla::AnimationCollection<mozilla::dom::CSSAnimation>*)

But then gets its timeline updated in a subsequent restyle but without removing from the pending animation tracker:

(rr) bt
#0  RefPtr<mozilla::dom::AnimationTimeline>::assign_assuming_AddRef(mozilla::dom::AnimationTimeline*)
    (this=0x7f9b155e5590, aNewPtr=0x7f9b15544a60)
    at /home/emilio/src/moz/gecko-6/obj-debug/dist/include/mozilla/RefPtr.h:68
#1  RefPtr<mozilla::dom::AnimationTimeline>::assign_with_AddRef(mozilla::dom::AnimationTimeline*)
    (this=0x7f9b155e5590, aRawPtr=0x7f9b15544a60)
    at /home/emilio/src/moz/gecko-6/obj-debug/dist/include/mozilla/RefPtr.h:62
#2  RefPtr<mozilla::dom::AnimationTimeline>::operator=(mozilla::dom::AnimationTimeline*)
    (this=0x7f9b155e5590, aRhs=0x7f9b15544a60)
    at /home/emilio/src/moz/gecko-6/obj-debug/dist/include/mozilla/RefPtr.h:190
#3  mozilla::dom::Animation::SetTimelineNoUpdate(mozilla::dom::AnimationTimeline*)
    (this=0x7f9b155e5500, aTimeline=0x7f9b15544a60) at /home/emilio/src/moz/gecko-6/dom/animation/Animation.cpp:257
#4  0x00007f9b218215aa in UpdateOldAnimationPropertiesWithNew(mozilla::dom::CSSAnimation&, mozilla::TimingParams&&, nsTArray<mozilla::Keyframe>&&, bool, mozilla::CSSAnimationProperties, ServoCSSAnimationBuilder&, mozilla::dom::AnimationTimeline*)
    (aOld=..., aNewTiming=..., aNewKeyframes=nsTArray<mozilla::Keyframe> && = {...}, aOverriddenProperties=mozilla::CSSAnimationProperties::None, aBuilder=..., aTimeline=0x7f9b15544a60, aNewIsStylePaused=<optimized out>)
    at /home/emilio/src/moz/gecko-6/layout/style/nsAnimationManager.cpp:182
#5  BuildAnimation(nsPresContext*, mozilla::NonOwningAnimationTarget const&, nsStyleUIReset const&, unsigned int, ServoCSSAnimationBuilder&, mozilla::AnimationCollection<mozilla::dom::CSSAnimation>*)
    (aPresContext=0x7f9b15541400, aTarget=..., aStyle=..., animIdx=0, aBuilder=..., aCollection=0x7f9b15572e00)
    at /home/emilio/src/moz/gecko-6/layout/style/nsAnimationManager.cpp:286
#6  BuildAnimations(nsPresContext*, mozilla::NonOwningAnimationTarget const&, nsStyleUIReset const&, ServoCSSAnimationBuilder&, mozilla::AnimationCollection<mozilla::dom::CSSAnimation>*, nsTBaseHashSet<nsRefPtrHashKey<nsAtom> >&)
    (aPresContext=0x7f9b15541400, aTarget=..., aStyle=..., aBuilder=..., aCollection=0x7f9b15572e00, aReferencedAnimations=...) at /home/emilio/src/moz/gecko-6/layout/style/nsAnimationManager.cpp:338
#7  nsAnimationManager::DoUpdateAnimations(mozilla::NonOwningAnimationTarget const&, nsStyleUIReset const&, ServoCSSAnimationBuilder&) (this=0x7f9b15596bf0, aTarget=..., aStyle=..., aBuilder=...)
    at /home/emilio/src/moz/gecko-6/layout/style/nsAnimationManager.cpp:398
#8  0x00007f9b21820c10 in nsAnimationManager::UpdateAnimations(mozilla::dom::Element*, mozilla::PseudoStyleType, mozilla::ComputedStyle const*)
    (this=0x7f9b155c87c0, aElement=<optimized out>, aPseudoType=<optimized out>, aComputedStyle=<optimized out>)
    at /home/emilio/src/moz/gecko-6/layout/style/nsAnimationManager.cpp:374
#9  0x00007f9b217e9b60 in Gecko_UpdateAnimations(mozilla::dom::Element const*, mozilla::ComputedStyle const*, mozilla::ComputedStyle const*, mozilla::UpdateAnimationsTasks)
    (aElement=0x1, aOldComputedData=0x0, aComputedData=0x7f9b155e44c8, aTasks=(mozilla::UpdateAnimationsTasks::CSSAnimations | mozilla::UpdateAnimationsTasks::EffectProperties | mozilla::UpdateAnimationsTasks::CascadeResults))
    at /home/emilio/src/moz/gecko-6/layout/style/GeckoBindings.cpp:522
#10 0x00007f9b254bc533 in style::gecko::wrapper::{impl#16}::update_animations

Boris do you know what the best way to deal with this is? Should we be removing from the pending tracker or just turn the assertion into an early return or something?

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53a129b09ca5
Use whether the embedder element intersects the viewport to decide whether to throttle in-process iframes. r=smaug

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acce626fe05a
Fix new test in XOrigin mode.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ce86fc8bdc3
Minor fix so this works for in-process frames in DevTools properly.

Comment 26

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #22)

Ok so the devtools tests I got figured out. Boris, it seems we're asserting rather frequently in scroll-animations/css/at-scroll-timeline-dynamic.tentative.html after my change, and I'm pretty sure it's not a bug in my code.

The assert is this:

      MOZ_ASSERT(timeline->IsMonotonicallyIncreasing(),
                 "Don't put non-MonotoniciallyIncreasing timeline into "
                 "PendingAnimationTracker");

Boris do you know what the best way to deal with this is? Should we be removing from the pending tracker or just turn the assertion into an early return or something?

This is our missing part. I think we could just remove the scroll-timeline animations from pending animation tracker when setting a new timeline if the new timeline is scroll-timeline and old timeline is document timeline. We play the scroll-timeline animations directly without touching pending animation tracker (which is only for document-timeline animations). In this test case, we switch between document-timeline and scroll-timeline, and obviously we forgot to remove it from pending animation tracker.

Note: animation-timeline: auto uses document-timeline, and animation-timeline: <custom-ident> uses scroll-timeline if we define it somewhere.

Comment 27

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/53a129b09ca5
https://hg.mozilla.org/mozilla-central/rev/acce626fe05a
https://hg.mozilla.org/mozilla-central/rev/7ce86fc8bdc3

Comment 28

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

I managed to reproduce this bug on a 2021-09-10 Nightly build on macOS 11 using the free shareware files linked from the site; verified as fixed on Firefox 102.0b8(20220614185842) and Nightly 103.0a1(20220614213729) on macOS 11, Windows 10 x64 and Ubuntu 20.04.