895700 - WOFF should be prioritized over images like JS and CSS

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Steve Workman [:sworkman] (INACTIVE)]

Currently JS and CSS loads from the network are prioritized over images - Bug 792438. WOFF should be prioritized similarly.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 2

[Patrick McManus [:mcmanus]]

I've validated this patch with webpagetest mobile and desktop profiles and the somewhat absurdist page: http://www.ducksong.com/misc/123.html which has a lot of images and several user font files that compete for the same bandwidth.

The difference is most notable on mobile - where the font download times drop from 13-28 seconds, down to a relatively consistent 8 seconds. The time to first render commensurately improves. speedindex drops from 130K to 98K

Its a little noisier on my desktop.. the fonts are not especially big, and the waterfall makes it look like there isn't too much bandwidth contention at font loading time for my bigger pipe.. so an average run doesn't change very much (the fonts take about 400ms either way to load and start that process around the 3000ms mark) - but after enough runs its clear that the ones with this patch exhibit less variability in the font loading time.. I suspect that is due to reduced packet loss during the font loading phase.

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 831248 [details] [diff] [review]
patch 0

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

::: layout/style/nsFontFaceLoader.cpp
@@ +377,5 @@
>      httpChannel->SetReferrer(aFontFaceSrc->mReferrer);
> +  nsCOMPtr<nsIHttpChannelInternal>
> +    internalHttpChannel(do_QueryInterface(channel));
> +  if (internalHttpChannel)
> +    internalHttpChannel->SetLoadAsBlocking(true);

I understand giving the channel higher priority, but do we really want this to be a blocking load?

@@ +380,5 @@
> +  if (internalHttpChannel)
> +    internalHttpChannel->SetLoadAsBlocking(true);
> +  nsCOMPtr<nsISupportsPriority> priorityChannel(do_QueryInterface(channel));
> +  if (priorityChannel)
> +    priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_HIGH);

One nit: please add braces to the if-statements, as per style guide.

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Comment on attachment 831248 [details] [diff] [review]
> patch 0
> 
> Review of attachment 831248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsFontFaceLoader.cpp
> @@ +377,5 @@
> >      httpChannel->SetReferrer(aFontFaceSrc->mReferrer);
> > +  nsCOMPtr<nsIHttpChannelInternal>
> > +    internalHttpChannel(do_QueryInterface(channel));
> > +  if (internalHttpChannel)
> > +    internalHttpChannel->SetLoadAsBlocking(true);
> 
> I understand giving the channel higher priority, but do we really want this
> to be a blocking load?
> 

the nomenclature is a little funny here - it upgrades the load to the blocking class - all things in the blocking class stop the initiation of things not in the blocking class.. basically the blocking class is now {head-js/css/fonts} because they (more or less) block the rendering of the page and experience has shown it isn't great to have those things fighting for bandwidth with images.

unfortunately http/1 doesn't have a real sense of priority other than queue order - and what we often see on then web is a ton of sharding of images that makes sure we don't queue much of anything. Bumping the priority helps when we are queueing, but the load-as-blocking thing helps with the sharding.

http/2/spdy is a lot nicer here - priorities are a lot more meaningful - and necko takes that into account (essentially not doing the blocking thing wrt other http/2 channels - it just uses priority for them).

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> (In reply to Jonathan Kew (:jfkthame) from comment #3)
> > Comment on attachment 831248 [details] [diff] [review]
> > patch 0
> > 
> > Review of attachment 831248 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/style/nsFontFaceLoader.cpp
> > @@ +377,5 @@
> > >      httpChannel->SetReferrer(aFontFaceSrc->mReferrer);
> > > +  nsCOMPtr<nsIHttpChannelInternal>
> > > +    internalHttpChannel(do_QueryInterface(channel));
> > > +  if (internalHttpChannel)
> > > +    internalHttpChannel->SetLoadAsBlocking(true);
> > 
> > I understand giving the channel higher priority, but do we really want this
> > to be a blocking load?
> > 
> 
> the nomenclature is a little funny here - it upgrades the load to the
> blocking class - all things in the blocking class stop the initiation of
> things not in the blocking class.. 

But this doesn't have any connection to blocking the initial reflow and rendering of the page, or firing the onload event? (Sorry to be slow, but I'm not really familiar with all this...)

Comment 6

[Patrick McManus [:mcmanus]]

internalHttpChannel->SetLoadAsBlocking(true) is an indicator to necko that the results of this channel block useful interpretation of the page. The exact definition of that is fuzzy, but it is a useful notion in the abstract when doing resource prioritization.

just as we try not to layout a page without css, and we can't lay it out without certain kinds of javascript, I think user fonts are pretty much in the same category - where a redraw with new font information is pretty traumatic from a usability perspective. This is pretty different than the images, or the xhrs, the flash, etc.. which can all bubble in with less trouble.

The notion is certainly closely related to priority, but we treat it as a separate attribute.. partly for pragmatic reasons that we don't have to track down the individual exact priority of every channel that is used to make sure it stays on the right side of the line :)

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> internalHttpChannel->SetLoadAsBlocking(true) is an indicator to necko that
> the results of this channel block useful interpretation of the page. The
> exact definition of that is fuzzy, but it is a useful notion in the abstract
> when doing resource prioritization.
> 
> just as we try not to layout a page without css, and we can't lay it out
> without certain kinds of javascript, I think user fonts are pretty much in
> the same category - where a redraw with new font information is pretty
> traumatic from a usability perspective. This is pretty different than the
> images, or the xhrs, the flash, etc.. which can all bubble in with less
> trouble.
> 

Currently, it's a deliberate design feature that @font-face loads -- which in some cases may be much larger than typical CSS or JS resources -- do NOT block layout or rendering of the document, or the firing of the onload event.

(In order to try to avoid the unwelcome "flash of unfonted text", an in-progress font load will cause the affected text runs to be invisible, unless too much time has elapsed, in which case we'll resort to rendering a fallback font in preference to keeping the text hidden indefinitely.)

It's still not clear to me whether the change here is ONLY "an indicator to necko" that will cause it to prioritize font loads ahead of certain other resources, or will ALSO affect gecko layout behavior (if initial reflow and/or onload event processing will be blocked until all "blocking" channels have finished).

Can you clarify this, or do we need to check with someone else to confirm that side of things?

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to Jonathan Kew (:jfkthame) from comment #7)

> Currently, it's a deliberate design feature that @font-face loads -- which
> in some cases may be much larger than typical CSS or JS resources -- do NOT
> block layout or rendering of the document, or the firing of the onload event.
> 

interesting. I find that kind of weird given what they do, but interesting! thanks.

> (In order to try to avoid the unwelcome "flash of unfonted text", an
> in-progress font load will cause the affected text runs to be invisible,
> unless too much time has elapsed, in which case we'll resort to rendering a
> fallback font in preference to keeping the text hidden indefinitely.)

> 
> It's still not clear to me whether the change here is ONLY "an indicator to
> necko" that will cause it to prioritize font loads ahead of certain other
> resources, or will ALSO affect gecko layout behavior (if initial reflow
> and/or onload event processing will be blocked until all "blocking" channels
> have finished).
> 
> Can you clarify this, or do we need to check with someone else to confirm
> that side of things?

it is definitely just a necko indicator - I implemented it on that side, so I know for certain! OTOH - obviously if necko defers images until after the font is loaded that will have the second order effect of delaying onload().

So I'm definitely not trying to change layout policy in this bug, I was just trying to help necko execute what I inferred the policy was. If you are happy knowing that js/css get first class treatment, and fonts travel in economy with all the media then we can just go ahead with the high priority bit and not the loadasblocking() flag. basically an economy-plus pseudo upgrade.. totally up to you, but I will point out that first-class handling of js/css turns out to be a pretty big deal..

Comment 9

[Patrick McManus [:mcmanus]]

(even just getting the priority high will help in an http/2 world and gives necko more info to play with if it wants to get really clever in http/1 - so that's a good piece of info.)

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 831248 [details] [diff] [review]
patch 0

Redirecting the review here to :roc, as I'm not comfortable that I understand all this sufficiently to know what's appropriate. See the discussion above.

I'd have no problem with the AdjustPriority(PRIORITY_HIGH) part of this; what I'm less sure about is SetLoadAsBlocking(true). Patrick says that "things in the blocking class stop the initiation of things not in the blocking class" - if this implies that a large/slow font download would significantly delay the download of a (perhaps much smaller) image that the page needs, I'm not sure that's such a good thing.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 831248 [details] [diff] [review]
patch 0

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

Personally I think we should treat fonts similarly to style sheets, because they affect layout in sometimes major ways.

They should definitely delay firing of the load event. Even images do that! I wasn't aware they didn't. That's grist for another bug I guess.

Comment 12

[Jonathan Kew [:jfkthame]]

Remember that font loads are often not even initiated until later, as we don't know whether we need any given @font-face resource until we run the font-matching algorithm on the text of an element that has the given font-family in its style.

So the behavior is quite different to images, and downloadable fonts are (for better or worse) treated as a somewhat secondary component of the overall page -- they're "stylistic polish" rather than actual content or essential functionality.

Comment 13

[Patrick McManus [:mcmanus]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/f459062ebda8

I landed just the priotization bit here at this time.

Based on the discussion I'd like to find a couple more use cases in the wild that have fonts and images compete and measure them with the full patch under different network conditions before pushing that part.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nzherald.co.nz uses custom fonts and a lot of images.

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> Remember that font loads are often not even initiated until later, as we
> don't know whether we need any given @font-face resource until we run the
> font-matching algorithm on the text of an element that has the given
> font-family in its style.

Good point.

> So the behavior is quite different to images, and downloadable fonts are
> (for better or worse) treated as a somewhat secondary component of the
> overall page -- they're "stylistic polish" rather than actual content or
> essential functionality.

Web developers are prone to developing unreasonable expectations :-).

I think at some point we'll have to find a way to start font loads earlier.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f459062ebda8