Closed Bug 1390223 Opened 7 years ago Closed 7 years ago

Experiment: Don't mark dynamically added <head> css (aka <link rel="stylesheet"> elements) as Leaders

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Version:
56 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: mayhemer, Assigned: kershaw)

References

Details

Attachments

(1 file, 1 obsolete file)

WIP - Dertermine whether a link element is created from parser or not
4.26 KB, patch
Details | Diff | Splinter Review 
This may block loading fonts while these css files don't block first paint, so no need to block other resource loads, we already painted on the screen and will do a reflow anyway.

Rather mark them as Unblocked if necessary.

We already do that for dynamically added <head> js files.

Example page: https://www.janbambas.cz/ where 5 web fonts are blocked for more than 100ms because of two dynamically added css scripts.

This could also be solved with marking font loads as Unblocked which is probably programmatically simpler.
Blocks: CDP
Priority: -- → P3
Attached patch Marking font loads as Unblocked (obsolete) — — Splinter Review 
Summary:
 - As comment#0 suggested, simply mark font loads as Unblocked.
Assignee: nobody → kechang
Status: NEW → ASSIGNED
Attachment #8918771 - Flags: review?(honzab.moz)
Comment on attachment 8918771 [details] [diff] [review]
Marking font loads as Unblocked

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

I'd like to also ask :heycam to take a look.

This patch tells necko not allowing font loads to be blocked by other loads, which means font loads could complete a bit earlier.
Could this change break any spec or existing code?

Thanks.
Attachment #8918771 - Flags: feedback?(cam)
Comment on attachment 8918771 [details] [diff] [review]
Marking font loads as Unblocked

I'm not sure what the effect of making font loads unblocked really is.  Could it cause other resources on the page to be blocked until fonts finish downloading?  Since fonts can take a long time to download, I don't know whether that's the right prioritisation.

But from a spec perspective, this should be fine.
Attachment #8918771 - Flags: feedback?(cam) → feedback+
Comment on attachment 8918771 [details] [diff] [review]
Marking font loads as Unblocked

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

I just found that we already have bug 1349177 about marking font loads as Unblocked. According to bug 1349177 comment #10, we need some analysis first.

So, cancel the review for now.
Attachment #8918771 - Flags: review?(honzab.moz)
This bug should first go via some preliminary assessment of performance impact.  It's experimental and is about not marking dynamically added <link stylesheet> elements as Unblocked instead of Leaders.
Summary: Don't mark dynamically added <head> css as Leaders → Experiment: Don't mark dynamically added <head> css (aka <link rel="stylesheet"> elements) as Leaders
Also note that bug 1349177 has not gone through a deeper per-analyzes too.  And I more and more then to wontfixing it, since fonts don't block first paint in most cases.

I want to experiment with this on a 2G or so connection first.
I've created a patch to determine if a link element is created from parser and also done some analysis for https://www.janbambas.cz/, but I can't see the same blocking as described in comment #0.

Honza, do you have other sample pages for me to analyze? Thanks.
Flags: needinfo?(honzab.moz)
The point is to not mark dynamically added css as Leaders.  The font load blocking in comment 0 is dependent on timing, your provider may serve resources faster.  You may need to build a synthetic page to demo that.
Flags: needinfo?(honzab.moz)
I think the reason you don't see it on my site is that the server has been updated to h2 between reporting this bug and now...  we let leaders, unblocked and followers be handled by h2 then.
(In reply to Honza Bambas (:mayhemer) from comment #10)
> I think the reason you don't see it on my site is that the server has been
> updated to h2 between reporting this bug and now...  we let leaders,
> unblocked and followers be handled by h2 then.

Thanks, after disabling h2, I can see this blocking.

I've also done a WPT test on https://www.janbambas.cz/ with mitm proxy enabled (for minimizing the variations caused by network latency). Here is the result:

+---------------------+--------+---------
|                     | before |  after |
+---------------------------------------+
|Page Load Time(ms)   | 3604.62| 3598.1 |
+---------------------------------------+
|Start Render Time(ms)| 2622.96| 2597.5 |
+---------------------------------------+
|Speed index          | 2784.78| 2766.58|
|---------------------+------------------

The result shows there is a bit improvement without marking dynamically added css scripts as leaders. I think the result is quite trustworthy, even though the improvement is not obvious. So, I still think we should give this bug a go.
What do you think, Honza?
Flags: needinfo?(honzab.moz)
Thanks for doing this!  The improvement is very small (my front page is relatively simple), but it still is an improvement for h1 loading pages.  How does it affect when h2 is on on the same page?  

Could you run your patch by few more pages known to add dynamic CSS?  I unfortunately don't have any by hand.  If finding them is too complicated, then just push to talos to check if we don't regress any tests we run as standard on each push to avoid backouts.  Thanks.
Flags: needinfo?(honzab.moz)
comment 12
Flags: needinfo?(kechang)
(In reply to Honza Bambas (:mayhemer) from comment #12)
> Thanks for doing this!  The improvement is very small (my front page is
> relatively simple), but it still is an improvement for h1 loading pages. 
> How does it affect when h2 is on on the same page?  
> 

I think this patch won't have any effect for h2 case, sine web fonts and css files are from different origins.
If I was right, the css and font transactions are dispatched at [1], which means that font loading will not be blocked by css.

[1] https://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/netwerk/protocol/http/nsHttpConnectionMgr.cpp#1510

> Could you run your patch by few more pages known to add dynamic CSS?  I
> unfortunately don't have any by hand.  If finding them is too complicated,
> then just push to talos to check if we don't regress any tests we run as
> standard on each push to avoid backouts.  Thanks.

I ran talos tests two times [2][3] to see if there is any regression.
Unfortunately, it seems to get worse for tp6_youtube cases on windows platform.

So, I think maybe we should wontfix this bug. What do you think, Honza?


[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fbba526ee37b13f1b1d6382d15e0dcac3a685287&newProject=try&newRevision=3d159d88d66a49f9ec1622f67836571ab82d90e9&framework=1
[3] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=9baea271aa127e0bb6b915c786455b366ae3ee7f&newProject=try&newRevision=1096731496c207e1c70f2ec7e582dcd402dc7b47&framework=1
Flags: needinfo?(kechang) → needinfo?(honzab.moz)
(In reply to Kershaw Chang [:kershaw] from comment #14)
> (In reply to Honza Bambas (:mayhemer) from comment #12)
> > Thanks for doing this!  The improvement is very small (my front page is
> > relatively simple), but it still is an improvement for h1 loading pages. 
> > How does it affect when h2 is on on the same page?  
> > 
> 
> I think this patch won't have any effect for h2 case, sine web fonts and css
> files are from different origins.
> If I was right, the css and font transactions are dispatched at [1], which
> means that font loading will not be blocked by css.

The blocking happens per page (request context) not per origin.  But we do an h2 dispatch before checking the leader/follower thing.  So you are probably right.  Still, I believe setting the Unblocked flag will change priorities within an h2 session, but only negligible.

> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 1c4901d060e3953e41088c038b62a0c026c1e1fb/netwerk/protocol/http/
> nsHttpConnectionMgr.cpp#1510
> 
> > Could you run your patch by few more pages known to add dynamic CSS?  I
> > unfortunately don't have any by hand.  If finding them is too complicated,
> > then just push to talos to check if we don't regress any tests we run as
> > standard on each push to avoid backouts.  Thanks.
> 
> I ran talos tests two times [2][3] to see if there is any regression.
> Unfortunately, it seems to get worse for tp6_youtube cases on windows
> platform.
> 
> So, I think maybe we should wontfix this bug. What do you think, Honza?

That is an interesting data point.  Yes, this could potentially block during the first-paint interval.  So, I think this is WONTFIX.

> 
> 
> [2]
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=fbba526ee37b13f1b1d6382d15e0dcac
> 3a685287&newProject=try&newRevision=3d159d88d66a49f9ec1622f67836571ab82d90e9&
> framework=1
> [3]
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=9baea271aa127e0bb6b915c786455b36
> 6ae3ee7f&newProject=try&newRevision=1096731496c207e1c70f2ec7e582dcd402dc7b47&
> framework=1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(honzab.moz)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: