boost load priority of preloaded image for <img> without height/width attribute

RESOLVED WONTFIX

Status

()

P3
normal
RESOLVED WONTFIX
a year ago
9 months ago

People

(Reporter: schien, Assigned: schien)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

In bug 1312770, I introduce a prioritization algorithm to increase the priority for image frame that doesn't have height/width information. However, it'll be too late because HTML5 parser will trigger the HTTP transaction way earlier than nsImageFrame initialization.

In order to further enforce this prioritization algorithm, we need to check height/width attribute while triggering image preload in HTML5 parser.
Comment hidden (mozreview-request)
verified using https://schien.github.io/testing/image-test/img-load-order.html

9 <img> in this test case:

<img src="./1hidden.png" style="visibility: hidden;"></img>
<img src="./2display-none.png" style="display: none;"></img>
<img src="./3hidden-size.png" style="visibility: hidden;" height="30" width="30"></img>
<img src="./4not-in-view.png" style="position: fixed; top: -80px; left: -80px"></img>
<img src="./5sure-not-in-view.png" style="position: fixed; top: -80px; left: -80px;" height="30" width="30"></img>
<img src="./6end-of-doc.png" style="position: absolute; top: 5000px;" height="30" width="30"></img>
<img src="./7known-size.png" height="30" width="30"></img>
<img src="./8no-size.png"></img>
<img src="./9zero-size.png" height="0" width="0"></img>

All the <img> without height or width attr will get extra priority. One thing to be noticed is this algorithm will also increase the priority for 2display-non.png, however it is not necessary due to "display: none".
As I described in comment #2, this solution will boost "display: none" image. @mayhemer you might want to try it first to see if it matches your expectation.
Flags: needinfo?(honzab.moz)

Comment 4

a year ago
mozreview-review
Comment on attachment 8888216 [details]
Bug 1382567 - boost load priority for <img> without height/width.

https://reviewboard.mozilla.org/r/159162/#review166380

::: parser/html/nsHtml5SpeculativeLoad.h:275
(Diff revision 1)
>       * If mOpCode is eSpeculativeLoadScript[FromHead], this is the value of the
>       * "integrity" attribute.  If the attribute is not set, this will be a void
>       * string.
>       */
>      nsString mIntegrity;
> +    bool mHasHeightWidth = false;

please add a comment who changes this and what effect this has and why.

::: parser/html/nsHtml5TreeBuilderCppSupplement.h:219
(Diff revision 1)
>          } else if (nsGkAtoms::video == aName) {
>            nsHtml5String url =
>              aAttributes->getValue(nsHtml5AttributeName::ATTR_POSTER);
>            if (url) {
>              mSpeculativeLoadQueue.AppendElement()->InitImage(
> -              url, nullptr, nullptr, nullptr, nullptr);
> +              url, nullptr, nullptr, nullptr, nullptr, false);

so this will add the priority as well?  do we want it in this case?

::: parser/html/nsHtml5TreeBuilderCppSupplement.h:263
(Diff revision 1)
>          if (nsGkAtoms::image == aName) {
>            nsHtml5String url =
>              aAttributes->getValue(nsHtml5AttributeName::ATTR_XLINK_HREF);
>            if (url) {
>              mSpeculativeLoadQueue.AppendElement()->InitImage(
> -              url, nullptr, nullptr, nullptr, nullptr);
> +              url, nullptr, nullptr, nullptr, nullptr, false);

as well here.

if not, the argument should be "raise priority" instead and in the <img> tag case pass !mHasWH?
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #3)
> As I described in comment #2, this solution will boost "display: none"
> image. @mayhemer you might want to try it first to see if it matches your
> expectation.

Thanks for the patch!  I think this is what we should try, but we may need a preference for it for WPT and potential pref studies (if applicable).
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8888216 [details]
Bug 1382567 - boost load priority for <img> without height/width.

https://reviewboard.mozilla.org/r/159162/#review166380

> please add a comment who changes this and what effect this has and why.

Comment is added.

> so this will add the priority as well?  do we want it in this case?

The size of poster image will not be used to determine the layout size of <video>. No need to increase the priority while loading poster.

> as well here.
> 
> if not, the argument should be "raise priority" instead and in the <img> tag case pass !mHasWH?

This is the embedded image in an SVG image, which doesn't affect the layout dimension of the entire <svg> tag. No need to increase the priority.
I'll change the variable name to `mShouldBoostPriority`.
Comment hidden (mozreview-request)
Do we have a story for why Web authors wouldn't start omitting width and height for hero images in the hope of gaming the preload priority?
:sc, please make also sure that the feature has a preference we can switch it off with, thanks.


(In reply to Henri Sivonen (:hsivonen) from comment #8)
> Do we have a story for why Web authors wouldn't start omitting width and
> height for hero images in the hope of gaming the preload priority?

Note that an image w/o width and height will cause a reflow of the page when loaded after the layout has been performed, so it will cause another performance issue.  We are trying to avoid it with this patch - we may put the image on wire a bit sooner but just because we want to avoid reflows.

This is experimental.  We may also very well find out that not setting the priority and doing a sequential (as it appears in the markup) preload - as we do now - delivers the image soon enough already.
Blocks: 1312741

Comment 10

a year ago
mozreview-review
Comment on attachment 8888216 [details]
Bug 1382567 - boost load priority for <img> without height/width.

https://reviewboard.mozilla.org/r/159162/#review170820

Given that this is largely about parser, hsivonen should review.
Attachment #8888216 - Flags: review?(bugs)
(In reply to Honza Bambas (:mayhemer) from comment #9)
> :sc, please make also sure that the feature has a preference we can switch
> it off with, thanks.
Do we want to use the pref "image.layout_network_priority" to control it or create a separate one?
https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/image/imgRequest.cpp#563

I was thinking about using single pref to switch the image prioritization algorithm while writing the patch.
Flags: needinfo?(honzab.moz)
image.layout_network_priority is OK to use, thanks.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #12)
> image.layout_network_priority is OK to use, thanks.

Cool! In this patch I use `imgRequest::BoostPriority` to adjust priority, which is controlled by this pref already.

Comment 14

a year ago
mozreview-review
Comment on attachment 8888216 [details]
Bug 1382567 - boost load priority for <img> without height/width.

https://reviewboard.mozilla.org/r/159162/#review172024

The width/height attributes in the markup is only one way an img can get a defined size. CSS can also be used to specify the size. However if you want to try this out to see if it helps we can do that.
Attachment #8888216 - Flags: review?(tnikkel) → review+
Attachment #8888216 - Flags: review?(hsivonen)
Comment hidden (mozreview-request)
(In reply to Honza Bambas (:mayhemer) from comment #9)
> :sc, please make also sure that the feature has a preference we can switch
> it off with, thanks.
> 
> 
> (In reply to Henri Sivonen (:hsivonen) from comment #8)
> > Do we have a story for why Web authors wouldn't start omitting width and
> > height for hero images in the hope of gaming the preload priority?
> 
> Note that an image w/o width and height will cause a reflow of the page when
> loaded after the layout has been performed, so it will cause another
> performance issue.

Yes, but authors may not realize that. Considering what tnikkel said in comment 14, doesn't this patch mean that authors have an incentive to declare width and height via CSS (via the style attribute even) and remove the width and height attributes? That is, can authors avoid the reflow-related perf issue by specifying width and height via CSS?

(The code looks OK, but not marking r+ yet in the sense of trying to understand if doing this gives Web authors bad incentives.)
(In reply to Henri Sivonen (:hsivonen) from comment #16)
> (In reply to Honza Bambas (:mayhemer) from comment #9)
> > :sc, please make also sure that the feature has a preference we can switch
> > it off with, thanks.
> > 
> > 
> > (In reply to Henri Sivonen (:hsivonen) from comment #8)
> > > Do we have a story for why Web authors wouldn't start omitting width and
> > > height for hero images in the hope of gaming the preload priority?
> > 
> > Note that an image w/o width and height will cause a reflow of the page when
> > loaded after the layout has been performed, so it will cause another
> > performance issue.
> 
> Yes, but authors may not realize that. Considering what tnikkel said in
> comment 14, doesn't this patch mean that authors have an incentive to
> declare width and height via CSS (via the style attribute even) and remove
> the width and height attributes? That is, can authors avoid the
> reflow-related perf issue by specifying width and height via CSS?
> 
> (The code looks OK, but not marking r+ yet in the sense of trying to
> understand if doing this gives Web authors bad incentives.)

To be honest, I have no way to confirm this would be a perf win at all.  I'm totally missing tools for most if not any CDP features we have invented.  So, let's put this aside for now for all these reasons.  I want to focus now more on inventing performance analyzes tools than trying to push something for any cost.
Priority: -- → P4
Attachment #8888216 - Flags: review?(hsivonen)
Attachment #8888216 - Flags: review+
Priority: P4 → P3
Unless confirmed this would be a win close to remove from the radar.
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → WONTFIX
Attachment #8888216 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.