Closed Bug 963505 Opened 6 years ago Closed 6 years ago

iframes should load with low network priority

Categories

(Core :: DOM: Navigation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: blassey, Assigned: blassey)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.
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.
Component: Networking → Layout
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...
Attached patch iframe_priority.patch (obsolete) — Splinter Review
Attachment #8365359 - Flags: review?(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?
Flags: needinfo?(blassey.bugs)
Component: Layout → Document Navigation
(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.
Flags: needinfo?(blassey.bugs)
Patrick?  You've been thinking about this sort of thing a lot recently, I'd like to know what you think.
Flags: needinfo?(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.
Flags: needinfo?(mcmanus)
Do we have any way to measure any possible improvements or regressions from this patch?
(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.
(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.
wlach, does the nytimes test have iframes?
(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)?
Flags: needinfo?(mcmanus)
(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.
(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
Flags: needinfo?(mcmanus)
bz, any suggestion for how to adjust the priority of the subresources?
Flags: needinfo?(bzbarsky)
Attached patch iframe_priority.patch (obsolete) — Splinter Review
adjusts priority by 1
Assignee: nobody → blassey.bugs
Attachment #8365359 - Attachment is obsolete: true
Attachment #8365359 - Flags: review?(bzbarsky)
Attachment #8370164 - Flags: review?(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).
Flags: needinfo?(bzbarsky)
Attached patch loadgroup priority (obsolete) — Splinter Review
does this look right? I haven't tested it yet
Attachment #8370569 - Flags: feedback?(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?
Attached patch loadgroup priority (obsolete) — Splinter Review
yup, forgot to qref. Sorry
Attachment #8370569 - Attachment is obsolete: true
Attachment #8370569 - Flags: feedback?(bzbarsky)
Attachment #8370853 - Flags: feedback?(bzbarsky)
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?
> 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 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.
Attachment #8370853 - Flags: feedback?(bzbarsky) → feedback+
Attachment #8370164 - Flags: review?(bzbarsky)
Attachment #8370164 - Attachment is obsolete: true
Attachment #8370853 - Attachment is obsolete: true
Attachment #8371257 - Flags: review?(bzbarsky)
Patrick, just wanted to confirm with you that adjusting the priority of the loadgroup will have the intended effect.
Flags: needinfo?(mcmanus)
Comment on attachment 8371257 [details] [diff] [review]
iframe_priority.patch

r=me
Attachment #8371257 - Flags: review?(bzbarsky) → review+
(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
Flags: needinfo?(mcmanus)
Blocks: 947390
https://hg.mozilla.org/mozilla-central/rev/40af3220c800
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 1509754
You need to log in before you can comment on or make changes to this bug.