Closed Bug 1145439 Opened 9 years ago Closed 9 years ago

Throttle off-screen iframes to be more efficient

Categories

(Core :: Graphics: Canvas2D, defect)

Product:
Core
Component:
Graphics: Canvas2D
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed
relnote-firefox --- -

People

(Reporter: bugs, Assigned: seth)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [shumway])

Attachments

(2 files, 4 obsolete files)

(Part 1) - Throttle requestAnimationFrame for non-visible iframes
19.70 KB, patch
Details | Diff | Splinter Review
(Part 2) - Make test_scroll_event_ordering.html wait for rAF to unthrottle
1.24 KB, patch
roc
: review+
Details | Diff | Splinter Review 
Similar to what we do for images in bug 689623, we need a way to reduce the performance impact of canvas instances that are off-screen. We already throttle down these instances when in a background tab but not when outside the viewport in the foreground tab.
OS: Mac OS X → All
Whiteboard: [shumway]
See Also: → 689623
Assignee: nobody → seth
So for this do we just want a property on the canvas element that checks if there is retained painting data associated with the frame, indicating that it was drawn last paint? Or something else?
(In reply to Timothy Nikkel (:tn) from comment #1)
> So for this do we just want a property on the canvas element that checks if
> there is retained painting data associated with the frame, indicating that
> it was drawn last paint? Or something else?

I think this is really about throttling RequestAnimationFrame delivery to <iframe>'s that aren't visible. The discussion of <canvas> is more about motivation for the change.

I talked to Mason and he suggested that if we can flag the document as visible or not visible, we can probably add code to do this in this neighborhood:

https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp#1536
Here's the patch.
Attachment #8586951 - Flags: review?(tnikkel)
Attachment #8586951 - Flags: review?(mchang)
Some notes on the patch:

I think in the long term using retained layer data might be a better approach, but I avoided it for this patch, because it seemed like it was going to be a bit trickier than expected.

A lot of the complexity in the refresh driver comes from using separate arrays for throttled frame requesting documents vs. non-throttled ones. I initially kept them in the same array, but this forced us to potentially do a significant amount of copying on every tick. This approach is also nice in that we can switch off the high precision timer if we only have throttled frame requesting documents, and we basically don't have to write any new code to make that happen.

I saw good improvements to CPU usage (and hence energy usage) in practice from this patch. Visiting areweflashyet.com in current Nightly and keeping the Shumway iframe in view, I saw 45% CPU usage from the content process and 16% from the chrome process. Scrolling the iframe offscreen dropped CPU usage to 35% and 3% respectively. With the patch applied, I see pretty much the same numbers with the Shumway iframe in view, but scrolling the iframe offscreen drops CPU usage down to 15% and 1.5% respectively. There's a lot of variance in these numbers, but it seems fair to say that this patch roughly halves CPU usage in both the content and chrome processes when the Shumway iframe is offscreen, which is a nice win!
Attachment #8586951 - Flags: review?(tnikkel) → review?(mstange)
(Switched from Timothy to Markus to reduce Timothy's review burden a little.)
Comment on attachment 8586951 [details] [diff] [review]
Throttle requestAnimationFrame for non-visible iframes.

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

Just to make sure I understand what this code is doing correctly. This patch adds a list of mThrottledFrameRequests that tick at the same rate as the inactive refresh driver timer's initial timer. (The inactive refresh timer backs off by 2x everytime, so with this patch, the throttled rAF have a constant throttle rate). If it's time to tick the throttled request frames, we move the documents from the throttled list to the unthrottled list and tick during "this" refresh tick. We also check current unthrottled frame requests and put them in the throttled list if necessary.

::: dom/base/nsDocument.cpp
@@ +3942,5 @@
> +    return false;  // Can't do anything smarter.
> +  }
> +
> +  if (!displayRootFrame->DidPaintPresShell(mPresShell)) {
> +    // We didn't get painted during the last paint, so throttle.

I'm confused by this. If we didn't get painted during the last paint, shouldn't we want to paint now? Or is this more like "this frame was already throttled last time, so keep on throttling?"

@@ +3947,5 @@
> +    return true;
> +  }
> +
> +  // We got painted during the last paint, so run at full speed.
> +  return false;

You should get someone else to review this part. I'm don't know if this is enough to detect that this document is not visible on the screen.

::: dom/base/nsIDocument.h
@@ +2104,5 @@
> +   * @return true if this document's frame request callbacks should be
> +   * throttled. We throttle requestAnimationFrame for documents which aren't
> +   * visible (e.g. scrolled out of the viewport).
> +   */
> +  bool ShouldThrottleFrameRequests();

nit: rename to ShouldThrottleInvisibleFrameRequests() or something like that.

::: layout/base/nsRefreshDriver.cpp
@@ +1570,5 @@
>        // This is the Flush_Style case.
>  
>        // Grab all of our frame request callbacks up front.
>        nsTArray<DocumentFrameCallbacks>
> +        frameRequestCallbacks(mFrameRequestCallbackDocs.Length() +

nit: I'd prefer we extract this out to a method like RunFrameRequestCallbacks(). nsRefreshDriver::Tick is getting too long imo.

@@ +1587,5 @@
> +          // enter high precision mode the next time the callback is scheduled.
> +          TakeFrameRequestCallbacksFrom(doc, frameRequestCallbacks);
> +
> +          // Removing the document from the array, so don't increment the index.
> +          mThrottledFrameRequestCallbackDocs.RemoveElementAt(i);

I'd prefer not having concurrent iteration / modification versus iterating and adding the docs to the unthrottled array then removing the docs from the throttled array.

::: layout/generic/nsIFrame.h
@@ +2828,5 @@
> +        return true;
> +      }
> +    }
> +    return false;
> +  }  

nit: delete whitespace.
Comment on attachment 8586951 [details] [diff] [review]
Throttle requestAnimationFrame for non-visible iframes.

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

nsIDocument::ShouldThrottleFrameRequests() looks good to me. It might need some additional comments or renamings that answer Mason's questions, though.
I don't really have any comments on the rest.

::: layout/generic/nsIFrame.h
@@ +2821,5 @@
> +   */
> +  bool DidPaintPresShell(nsIPresShell* aShell)
> +  {
> +    nsTArray<nsWeakPtr>* list = PaintedPresShellList();
> +    for (int i = 0, l = list->Length(); i < l; i++) {

We have ranged for loops nowadays, so I think this can just become
for (nsWeakPtr* item : *PaintedPresShellList()) {
  nsCOMPtr<nsIPresShell> shell = do_QueryReferent(item);
...
Attachment #8586951 - Flags: review?(mstange) → review+
or nsWeakPtr& actually.
Thanks for the reviews, folks!

(In reply to Mason Chang [:mchang] from comment #7)
> Just to make sure I understand what this code is doing correctly. This patch
> adds a list of mThrottledFrameRequests that tick at the same rate as the
> inactive refresh driver timer's initial timer. (The inactive refresh timer
> backs off by 2x everytime, so with this patch, the throttled rAF have a
> constant throttle rate).

Yeah. I'm planning to implement the exponential backoff in a followup bug, but it requires keeping per-document state which will add considerable complexity, so I'd prefer to avoid it in the initial implementation.

> If it's time to tick the throttled request frames,
> we move the documents from the throttled list to the unthrottled list and
> tick during "this" refresh tick. We also check current unthrottled frame
> requests and put them in the throttled list if necessary.

Well, we grab the requests of throttled request frames when we tick them, or if they shouldn't be throttled anymore. We don't move them into the unthrottled list, because we're about to call the callbacks anyway, and we only call the callbacks once. But when they call requestAnimationFrame() again, they'll end up in the unthrottled list, assuming they're still visible.

> I'm confused by this. If we didn't get painted during the last paint,
> shouldn't we want to paint now? Or is this more like "this frame was already
> throttled last time, so keep on throttling?"

This is a visibility test. If we didn't get painted during the last paint, it means we're not visible. That in turn means that we should throttle requestAnimationFrame callbacks.

> You should get someone else to review this part. I'm don't know if this is
> enough to detect that this document is not visible on the screen.

Yeah, Markus is reviewing that stuff.

> nit: rename to ShouldThrottleInvisibleFrameRequests() or something like that.

Well, this is done on a document level - either the document is visible or it isn't, and we throttle all frame requests for the document if it's invisible. In other words, ShouldThrottleInvisibleFrameRequests() should always return true. =) The question is just whether the document is visible. (At least for now.. we may add other criteria in the future.)

> nit: I'd prefer we extract this out to a method like
> RunFrameRequestCallbacks(). nsRefreshDriver::Tick is getting too long imo.

Sounds good!

> @@ +1587,5 @@
> I'd prefer not having concurrent iteration / modification versus iterating
> and adding the docs to the unthrottled array then removing the docs from the
> throttled array.

I'm not totally sure I understand what you're proposing, so correct me if I'm wrong, but I think we unfortunately can't do that. For the unthrottled case we can, of course, because we call all the callbacks every time. But in the unthrottled case, on most ticks we'd only grab the callbacks from a subset of the unthrottled documents - the ones that became visible since the last paint, meaning that we don't want to throttle them anymore. We could potentially copy the still-invisible documents into a second array, then clear the original array and copy them back, but that seems like a lot of copying for the common case where nothing or very little has changed.
Attachment #8586951 - Attachment is obsolete: true
Attachment #8586951 - Flags: review?(mchang)
This new version of the patch addresses all of the requested review changes so
far:

- I pulled the frame request callback handling into a separate method,
  nsRefreshDriver::RunFrameRequestCallbacks().

- I stopped doing concurrent modification of mThrottledFrameRequestCallbackDocs
  and switched to a theoretically slower but less footgun-y approach. Mason and
  I discussed this on IRC and agreed that in practice, the number of documents
  we're removing on a given tick will virtually always be zero or very small, so
  this is unlikely to make any performance difference in practice.

- I added a comment to nsDocument::ShouldThrottleFrameRequests() documenting the
  fact that we can drop an rAF frame when a previously-offscreen iframe is
  scrolled into view.

- I switched to the range-style for loop in nsIFrame::DidPaintPresShell(). I
  also made the switch in a couple of other places.

I also made one other change which wasn't requested by review:

- We now always run the callbacks for the documents in
  mFrameRequestCallbackDocs, and don't bother to check whether the documents
  have become throttled since the callbacks were added. This will cause us to
  run at most one extra normal-speed rAF callback when an iframe scrolls out of
  view, which doesn't seem like a problem. The code is both cleaner and more
  efficient since we don't have to check whether every document in
  mFrameRequestCallbackDocs has become throttled.
Attachment #8587917 - Flags: review?(mchang)
(In reply to Seth Fowler [:seth] from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a3152e63bd8

Looks pretty good.
Comment on attachment 8587917 [details] [diff] [review]
Throttle requestAnimationFrame for non-visible iframes

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

LGTM! Thanks for the changes.

::: layout/base/nsRefreshDriver.h
@@ +362,5 @@
>    // The most recently completed transaction id.
>    uint64_t mCompletedTransaction;
>  
>    uint32_t mFreezeCount;
> +

nit: delete newlines around mThrottledFrameRequestInterval.

::: layout/generic/nsIFrame.h
@@ +2825,5 @@
> +        return true;
> +      }
> +    }
> +    return false;
> +  }  

nit: delete trailing whitespace.
Attachment #8587917 - Flags: review?(mchang) → review+
Thanks for the review! Here's the final version. I tweaked the whitespace and
added one more comment.
(In reply to Wes Kocher (:KWierso) from comment #17)
> I had to back this out for Gu orange:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4796b5a120
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=8472516&repo=mozilla-
> inbound

Argh, sorry, I see now that failed on the try push too and I totally missed it.
Flags: needinfo?(seth)
Guillaume, it looks like the problem is here:

https://github.com/mozilla-b2g/gaia/blame/f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f/shared/js/font_size_utils.js#L147-L152

What we're doing in this bug is throttling requestAnimationFrame delivery to offscreen documents and iframes; when offscreen, requestAnimationFrame will only tick at 1 FPS for these documents.

Tests in https://github.com/mozilla-b2g/gaia/blob/master/apps/sharedtest/test/unit/font_size_utils_test.js start failing with this change. The failing tests appear to be more or less the same set as the tests using lazyLoad().

I don't understand the details of how these tests work, but presumably what's causing this failure is that these tests depend on the requestAnimationFrame callback having run, but they don't wait for it specifically before checking their results. In the test environment, the relevant document must be offscreen or non-visible (maybe the test window is zero size?), so the throttling causes a failure.

I don't think this should be too tough to fix, but being unfamiliar with this code and this test framework I don't know what the best approach to fix it is. Can we can modify these tests to fix this? Or would it be better to find an approach other than rAF to implement the code in font_size_utils.js?
Flags: needinfo?(gmarty)
I thought this code and tests were deprecated in favour of https://github.com/gaia-components/font-fit and https://github.com/gaia-components/gaia-header, but it seems that many apps in Gaia (email, settings...) are still using it.

Wilson, can you tell us if there are plans to move to gaia-header and get completely rid of font_size_utils.js? If not, do you know how much effort that would be?
If too much work is required, then we'll have to fix the test or update the code.
Flags: needinfo?(gmarty) → needinfo?(wilsonpage)
A quick fix for the tests could be to add this to a "setup" function:

  this.sinon.stub(window, 'requestAnimationFrame').yields()

(untested)
(In reply to Julien Wajsberg [:julienw] (PTO April 6th) from comment #21)
> A quick fix for the tests could be to add this to a "setup" function:
> 
>   this.sinon.stub(window, 'requestAnimationFrame').yields()


Julien, thanks for the suggestion! I pushed a try job to see if that works, linked in comment 22.

(In reply to Guillaume Marty [:gmarty] from comment #20)
> Wilson, can you tell us if there are plans to move to gaia-header and get
> completely rid of font_size_utils.js? If not, do you know how much effort
> that would be?

Thanks for looking into this, Guillaume!

What I'm thinking is: unless it's trivial to move to gaia-header, we should probably go with some sort of workaround in the short term.
Attachment #8587917 - Attachment is obsolete: true
(In reply to Seth Fowler [:seth] from comment #25)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=625377d051e2

Looks like the test change Julien suggested is working!
`headers.css` and `font_size_utils.js` have been deprecated. I'm guessing they were used by someone who didn't know <gaia-header> existed. I'd like to remove them from shared/ but I know that Email is incapable of using components that use shadow-dom. Perhaps we can move the files directly into `apps/email/`?

It should be super trivial to migrate the old building-block to <gaia-header>. Usually it just requires some markup changes and some small JS selector changes.
Flags: needinfo?(wilsonpage)
Depends on: 1152474
Bummer. I didn't hit that one on try.

I took a look at the test and I'm not sure why we'd throttle requestAnimationFrame in this case, but my best guess is that it's getting throttled because the JS in the test doesn't wait for the load event, and may be running before paint suppression ends.

I'm going to push a try job with a variation of the test that waits for the load event. Let's see if it still happens in that case.
OK, it looks like that change was indeed sufficient to get rid of the failures. I'll upload a patch that tweaks the test.
Attachment #8588091 - Attachment is obsolete: true
Updated the commit message of part 1.
Here's the patch. It's a trivial change; we just wait for the load event before
starting the test. As mentioned above, I'm guessing the reason we were
throttling requestAnimationFrame for this test is that we were running the JS
code before paint suppression ended. The try job in comment 31 confirms that
this change does fix the failure.
Attachment #8590982 - Flags: review?(roc)
Comment on attachment 8590982 [details] [diff] [review]
(Part 2) - Make test_scroll_event_ordering.html wait for the load event

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

I'm confused. SimpleTest.waitForFocus already says:
/**
 * If the page is not yet loaded, waits for the load event. In addition, if
 * the page is not yet focused, focuses and waits for the window to be
 * focused.
Attachment #8590982 - Flags: review?(roc)
Depends on: 1155320
Attachment #8590982 - Attachment is obsolete: true
OK, I figured out how to reproduce this issue locally. (It didn't happen on my
machine unless I slowed things down by inserting a bunch of printfs.)

The cause is indeed just an unfortunate event order - the first time we try to
run the rAF callback in this test, we haven't painted the document yet. That
means the document is in the throttled state, and we don't invoke the rAF
callback. We do paint it immediately afterwards, so it becomes unthrottled right
away, but by the time we get around to the next tick and call the rAF callback,
we've already called onScroll() and failed the test.

It's true that SimpleTest.waitForFocus also waits for onload; presumably waiting
for onload ourselves and then calling it just changed the event order in a
favorable way. I think there's a more robust way to fix the problem, though:
just don't start the test until we've received an rAF callback.  We'll get the
rAF callback on the tick after the document gets painted. At that point the
document has become unthrottled and stays unthrottled, and the test works fine.

This new version of the patch implements that approach. It completely fixes the
problem for me locally.
Attachment #8595055 - Flags: review?(roc)
Thanks for the review! Try looks good as well, so I think we're ready to try pushing this again.
I suspect this should be mentioned to developers in Firefox 40 for developers, as well as (possibly) on the requestAnimationFrame() page. I'm adding dev-doc-needed for that reason; if there's a strong reason not to mention it, please let me know.
Keywords: dev-doc-needed
Depends on: 1158557
Summary: Throttle off-screen canvas regions to be more efficient → Throttle off-screen iframes to be more efficient
Release Note Request (optional, but appreciated)
[Why is this notable]: improves performance and battery life on many pages
[Suggested wording]: Improve performance by throttling animations in frames that are not visible
[Links (documentation, blog post, etc)]: (dev doc see comment 41)
relnote-firefox: --- → ?
Too late for 40 :/
relnote-firefox: ? → -
Depends on: 1195592
Keywords: site-compat
Depends on: 1190093
This causes an issue with GWT (Google Web Toolkit) and indeed any javascript that animates a canvas on the main page from within an invisible iframe.

GWT javascript runs in an invisible iframe that gets created by GWT when the page loads:

<iframe src='javascript:""' id="my_gwt_demo" tabindex="-1" style="position: absolute; width: 0px; height: 0px; border: none; left: -1000px; top: -1000px;"></iframe>

A GWT application may use a canvas and may use requestAnimationFrame to animation it. This would work fine in all browsers, but causes issues with Firefox because requestAnimationFrame is coming from the iframe and so the resulting framerate from the requestAnimationFrame call is throttled.

Here is a demo of a webgl game written using GWT: http://cubris.eternaltechnics.com/
You'll see the problem in Firefox until you use the developer tools to change the GWT iframe to become visible (width and height of 1 and left and top of 0).

Works fine and smoothly in Chrome and Edge (with no jitter or screen tearing for that matter).

Can we change this implementation so that requestAnimationFrame calls from scripts within invisible iframes only get throttled if that iframe contains a canvas element? Or alternatively have some way of disabling this throttling feature.
Seth, looks like we need to tweak the heuristics for this some more.
Flags: needinfo?(seth)
Hi. Is there any update on this?
Hi, in Chromium we just have implemented something similar with one difference that might help with the class of issues mentioned here: the throttling is only for cross origin iframes. 

It's currently behind our experimental flag but we are considering enabling it by default very soon.

It would be great for the developers if the behavior was consistent across browsers.
I'm curious to hear feedback/concerns on our approach and whether or not Mozilla would consider it.

Design doc:
https://docs.google.com/document/d/1Dd4qi1b_iX-OCZpelvXxizjq6dDJ76XNtk37SZEoTYQ/edit
@ Kenji: thanks for the link to your design doc. The Microsoft Edge team is also thinking about similar iframe interventions: https://github.com/WICG/interventions/issues/9

@ Seth: do you have any updates on the priority inversion problem described in comment 46, where a throttled off-screen iframe tries to animate on-screen content? We should probably fork this issue into a separate, blocking bug.
Flags: needinfo?(seth)
Hi. Any update on this? Should we file a separate bug?
I'd hate to recommend to GWT developers to implement an 'if Firefox, do X' approach, when we should be avoiding that kind of thing.
Depends on: 1496899
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: