963505 - iframes should load with low network priority

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

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

I'm making the assumption that most things in iframes are not the primary content users are trying to get to (ads, like buttons etc), if we shuffle those sort of items later in the load priority we can get users to their content faster.

Comment 1

[Jason Duell]

I'm moving this this to Core::Layout, because I think the reviewer for this is going to be there (or at least somewhere other then Networking)

The idea sounds great on the networking end (lower priority items should indeed have lower priority! :).  It's the assumption that iframe = lower priority that I don't feel qualified to pass judgement on.

Comment 2

[(no longer active)]

Hmm, I think this depends on the websites.  A lot of very popular websites (examples: Gmail and Facebook at least on desktop) use iframes to implement parts of their functionality.  I would be much more comfortable if we did this at least for cross-origin iframes only...

Comment 3

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

Attachment: iframe_priority.patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Some questions.

1)  This is adjusting the priority of the iframe load itself, but not of subresources in that subframe.  Should we adjust the latter too?

2)  Thoughts on comment 2?

Comment 5

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

(In reply to Boris Zbarsky [:bz] from comment #4)
> Some questions.
> 
> 1)  This is adjusting the priority of the iframe load itself, but not of
> subresources in that subframe.  Should we adjust the latter too?
This wasn't intentional, but now that you point it out that seems best. My intention here isn't to permanently put the brakes on iframes, but just to get the main page loaded faster. Once the main iframe page gets loaded, presumably the main page's initial resources will have been loaded.

> 
> 2)  Thoughts on comment 2?

I don't share Ehsan's concern.  This will just prioritize loading the iframe below the resources of the main page, rather than let the ordering be the luck of the draw as it is currently.


One other note, this should probably be PRIORITY_LOW rather than PRIORITY_LOWEST.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Patrick?  You've been thinking about this sort of thing a lot recently, I'd like to know what you think.

Comment 7

[Patrick McManus [:mcmanus]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Patrick?  You've been thinking about this sort of thing a lot recently, I'd
> like to know what you think.

It sounds like all the things in the iframe should be +1 what they would be in the base frame. (+1 in the nice sense, which is actually lower priority). We generally have an ordered queue - so this will do the ordering you want.. it won't just create a slightly unbalanced round robin.

Dropping it from normal to low (a 10 point shift) could end up putting them in the image mix, which is a sure way to get them pretty much obliterated.

as I've said a few times the priority setting doesn't actually turn into a heck of a lot yet, but I hope to make it more impactful and the first step there is getting resources labeled right - so thanks for the bug. I support a change.

Comment 8

[(no longer active)]

Do we have any way to measure any possible improvements or regressions from this patch?

Comment 9

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> Do we have any way to measure any possible improvements or regressions from
> this patch?

we have eideticker to measure end-to-end page load time ("time to stable frame") over an emulated 3G network. That should give us reasonable confidence that we're not regressing the load of nytimes at least.

Comment 10

[(no longer active)]

(In reply to comment #9)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> > Do we have any way to measure any possible improvements or regressions from
> > this patch?
> 
> we have eideticker to measure end-to-end page load time ("time to stable
> frame") over an emulated 3G network. That should give us reasonable confidence
> that we're not regressing the load of nytimes at least.

Assuming nytimes is an interesting test case, sounds good.

Comment 11

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

wlach, does the nytimes test have iframes?

Comment 12

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

(In reply to Patrick McManus [:mcmanus] from comment #7)
> (In reply to Boris Zbarsky [:bz] from comment #6)
> > Patrick?  You've been thinking about this sort of thing a lot recently, I'd
> > like to know what you think.
> 
> It sounds like all the things in the iframe should be +1 what they would be
> in the base frame. (+1 in the nice sense, which is actually lower priority).
> We generally have an ordered queue - so this will do the ordering you want..
> it won't just create a slightly unbalanced round robin.
> 
> Dropping it from normal to low (a 10 point shift) could end up putting them
> in the image mix, which is a sure way to get them pretty much obliterated.
What do you suggest then? (PRIORITY_LOW - 1)? (PRIORITY_NORMAL + 1)? ((PRIORITY_LOW + PRIORITY_NORMAL) / 2)?

Comment 13

[William Lachance (:wlach)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #11)
> wlach, does the nytimes test have iframes?

Not sure exactly how to tell if the end result that we render has any iframes in it, but I just checked and the base html file that we load does not.

Comment 14

[Patrick McManus [:mcmanus]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #12)
> What do you suggest then? 

I suggest OldPriority() + 1

(Ideally this is true of everything in the iframe, which have a variety of different OldPriority()'s

Comment 15

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

bz, any suggestion for how to adjust the priority of the subresources?

Comment 16

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

Attachment: iframe_priority.patch

adjusts priority by 1

Comment 17

[Boris Zbarsky [:bzbarsky]]

> bz, any suggestion for how to adjust the priority of the subresources?

Sure.  What you actually want is to just adjust the priority of the subframe's loadgroup.  Probably at the point when the docshell's parent is first set or something (check IsFrame() at that point).

Comment 18

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

Attachment: loadgroup priority

does this look right? I haven't tested it yet

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8370569 [details] [diff] [review]
loadgroup priority

This looks identical to the previous patch...  Forgot to qref or git add or whatnot?

Comment 20

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

Attachment: loadgroup priority

yup, forgot to qref. Sorry

Comment 21

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

confirmed that the load group's priority is being adjusted correctly, but the channel priorities don't change as a result. Do we combine the two priorities in Necko?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Do we combine the two priorities in Necko?

Yes.  nsLoadGroup::AddRequest does:

544     if (mPriority != 0)
545         RescheduleRequest(request, mPriority);

and that basically does request->AdjustPriority(mPriority).  The effect is that all requests in the loadgroup are bumped by the priority of the loadgroup.  You should be able to see this if you examine the request after AsyncOpen returns.

Looking at patch now.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8370853 [details] [diff] [review]
loadgroup priority

mLoadGroup should never be null.

You probably want to use IsFrame() for the check you're currently doing with GetSameTypeRootTreeItem.

This will double-bump the priority if you move a docshell across parents (think bfcache traversal to a page with subframes).  What you probably want to do is check IsFrame() before nsDocLoader::SetDocLoaderParent and if so bump prioriy by -1, then check it again after you're set the parent and bump it by +1 as here.

Other than those nits, looks good.

Comment 24

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

Attachment: iframe_priority.patch

Comment 25

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

Patrick, just wanted to confirm with you that adjusting the priority of the loadgroup will have the intended effect.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8371257 [details] [diff] [review]
iframe_priority.patch

r=me

Comment 27

[Patrick McManus [:mcmanus]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #25)
> Patrick, just wanted to confirm with you that adjusting the priority of the
> loadgroup will have the intended effect.

loadgroups ftw

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/40af3220c800

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40af3220c800