Closed Bug 1363059 Opened 7 years ago Closed 7 years ago

Investigate icons that load during startup but aren't displayed

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jwatt, Assigned: johannh)

References

(Depends on 1 open bug)

Details

(Keywords: perf, Whiteboard: [reserve-photon-performance])

Attachments

(2 files)

patch that will log-image-creation to the console
1.50 KB, patch
Details | Diff | Splinter Review
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.
59 bytes, text/x-review-board-request
florian
: review+
jwatt
: review+
Details 
In addition to the icon covered by bug 1363040, when I load Firefox and just display the start page I see various other icons loading that are not actually displayed. We should investigate whether we can and should avoid loading these icons and spin off bugs as appropriate.


chrome://browser/skin/toolbarbutton-dropdown-arrow.png

  I thought this was part of the magnifying glass in the search
  field, but that doesn't seem to be the case since the arrow
  there is baked into the magnifying glass icon in
  chrome://browser/skin/search-indicator@2x.png


chrome://browser/skin/places/bookmarksToolbar@2x.png

  This doesn't seem to be the icon we use, even after clicking
  on the bookmark icon (this has rounded corners on the right
  hand side, but the one we use seems to be joined to the
  bookmarks menu button without the corners at the join
  being rounded.


chrome://browser/skin/places/toolbarDropMarker.png
chrome://browser/skin/places/unfiledBookmarks.png
chrome://mozapps/skin/places/defaultFavicon@2x.png
chrome://global/skin/icons/chevron@2x.png
chrome://global/skin/icons/autoscroll.png
chrome://global/skin/tree/folder@2x.png
chrome://branding/content/identity-icons-brand.svg
resource://gre-resources/loading-image.png
resource://gre-resources/broken-image.png

  These don't seem to be displayed


chrome://pocket/content/panels/img/pocketmenuitem16@2x.png

  Seems like this should only be displayed once the
  Pocket button is actually clicked. (And it seems like we
  should be using chrome://pocket-shared/skin/pocket.svg
  with an appropriate -moz-context-properties fill rather
  than this PNG icon.)


chrome://browser/skin/search-arrow-go.svg#search-arrow-go-inverted

  The default them uses the non-inverted icon, so it seems
  like we shouldn't be loading this inverted one before
  then going on to load the non-inverted variant
  chrome://browser/skin/search-arrow-go.svg#search-arrow-go
Florian, is this qf/photon-perf fodder?
Flags: needinfo?(florian)
(In reply to :Gijs from comment #1)
> Florian, is this qf/photon-perf fodder?

Could be :-). Although it could also be taken care of by the photon team cleaning up our CSS.

Jonathan, how did you build this list? I'm asking because I'm wondering if there's any way we could automate that process in a test, so that new unused images don't get added.
Blocks: photon-perf-upforgrabs
Flags: needinfo?(florian) → needinfo?(jwatt)
I used this patch.
Flags: needinfo?(jwatt)
Do you have the bookmark toolbar hidden when opening the browser?
> chrome://branding/content/identity-icons-brand.svg

This one is displayed in the identity block on about:home
(In reply to Johann Hofmann [:johannh] from comment #4)
> Do you have the bookmark toolbar hidden when opening the browser?

Yes. (That's the default for a new profile.)

(In reply to Johann Hofmann [:johannh] from comment #5)
> > chrome://branding/content/identity-icons-brand.svg
> 
> This one is displayed in the identity block on about:home

Ah, so it is.
(In reply to Jonathan Watt [:jwatt] from comment #3)
> Created attachment 8869054 [details] [diff] [review]
> patch that will log-image-creation to the console
> 
> I used this patch.

This seems to cover the first half of the problem (building the list of images we load), but what about the other half (deciding if an image has been displayed)?
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [reserve-photon-performance]
(In reply to Jonathan Watt [:jwatt] from comment #6)
> (In reply to Johann Hofmann [:johannh] from comment #4)
> > Do you have the bookmark toolbar hidden when opening the browser?
> 
> Yes. (That's the default for a new profile.)

Huh, that's bad then. I'm not very familiar with the bookmarks toolbar, does anyone know why so many bookmark toolbar icons have to load without the toolbar even showing? It's at least:

chrome://browser/skin/places/toolbarDropMarker.png
chrome://browser/skin/places/bookmarksToolbar@2x.png (which is just the placeholder for customize mode afaik)
chrome://browser/skin/places/unfiledBookmarks.png
chrome://global/skin/icons/chevron@2x.png
chrome://global/skin/tree/folder@2x.png
(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> This seems to cover the first half of the problem (building the list of
> images we load), but what about the other half (deciding if an image has
> been displayed)?

I did that manually via visual inspection which is most directly what we care about. To automate that it may be possible to hack into VectorImage::Show for SVG images and perhaps RasterImage::LookupFrameInternal/RasterImage::GetFrameInternal/RasterImage::DrawInternal for raster images (I'm less familiar with the latter).
(In reply to Johann Hofmann [:johannh] from comment #8)
> (In reply to Jonathan Watt [:jwatt] from comment #6)
> > (In reply to Johann Hofmann [:johannh] from comment #4)
> > > Do you have the bookmark toolbar hidden when opening the browser?
> > 
> > Yes. (That's the default for a new profile.)
> 
> Huh, that's bad then. I'm not very familiar with the bookmarks toolbar, does
> anyone know why so many bookmark toolbar icons have to load without the
> toolbar even showing? It's at least:
> 
> chrome://browser/skin/places/toolbarDropMarker.png
> chrome://browser/skin/places/bookmarksToolbar@2x.png (which is just the
> placeholder for customize mode afaik)
> chrome://browser/skin/places/unfiledBookmarks.png
> chrome://global/skin/icons/chevron@2x.png
> chrome://global/skin/tree/folder@2x.png

I suspect it's because the toolbar starts off collapsed rather than hidden? Which I think means layout still runs on it and presumably that's why we load all the images, too... assuming we don't load images for elements with display: none, anyway. You could test this hypothesis fairly easily by adding hidden=true to the toolbar, running ./mach build faster, and seeing if those files still show up as being loaded immediately.

If so, it's possible we could try to come up with some way of lazily initializing the toolbar and keeping it hidden=true initially.
Flags: qe-verify? → qe-verify-
I'm looking into making a test for this.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P3 → P1
I haven't fully fine-tuned the whitelist yet but it's far enough to get review, I think.
Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

https://reviewboard.mozilla.org/r/149890/#review154560

r+ for the platform changes. I assume florian will review the browser changes.

::: image/ImageFactory.cpp:100
(Diff revision 1)
> +#ifdef DEBUG
> +  // Record the image load for performance testing.
> +  if (NS_IsMainThread()) {
> +    nsAutoCString spec;
> +    aURI->GetSpec(spec);
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();

It would be cleaner if you put the spec fetching code in the |if|.

::: image/ImageFactory.cpp:101
(Diff revision 1)
> +  // Record the image load for performance testing.
> +  if (NS_IsMainThread()) {
> +    nsAutoCString spec;
> +    aURI->GetSpec(spec);
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    if (obs) {

Make that:

  if (NS_WARN_IF(obs)) {

::: image/ImageFactory.cpp:102
(Diff revision 1)
> +  if (NS_IsMainThread()) {
> +    nsAutoCString spec;
> +    aURI->GetSpec(spec);
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    if (obs) {
> +      obs->NotifyObservers(nullptr, "image-loaded", NS_ConvertUTF8toUTF16(spec).get());

I think this should be "image-loading" or "image-load-started". "loaded" is past tense, which implies that the image has finished loading whereas we're only just kicking off the load here.

::: image/RasterImage.cpp:1409
(Diff revision 1)
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    nsCOMPtr<nsIURI> imageURI = mURI->ToIURI();
> +    nsAutoCString spec;
> +    imageURI->GetSpec(spec);
> +    if (obs) {

My 'spec' and NS_WARN_IF comment above apply here too.

::: image/RasterImage.cpp:1410
(Diff revision 1)
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    nsCOMPtr<nsIURI> imageURI = mURI->ToIURI();
> +    nsAutoCString spec;
> +    imageURI->GetSpec(spec);
> +    if (obs) {
> +      obs->NotifyObservers(nullptr, "image-shown", NS_ConvertUTF8toUTF16(spec).get());

And "shown" is past tense, which again is wrong since we're just starting the draw here. Maybe call it "image-drawing" or "image-draw-started".

::: image/VectorImage.cpp:1035
(Diff revision 1)
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    nsCOMPtr<nsIURI> imageURI = mURI->ToIURI();
> +    nsAutoCString spec;
> +    imageURI->GetSpec(spec);
> +    if (obs) {

My 'spec', NS_WARN_IF and "shown" comments above apply here too.
Attachment #8878561 - Flags: review?(jwatt) → review+
Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

https://reviewboard.mozilla.org/r/149890/#review155020

r=me, but feel free to rerequest review (with a needinfo I guess) if the changes turn out to be more significant than you are comfortable carrying forward a review for.

::: browser/base/content/test/performance/browser_startup_images.js:12
(Diff revision 1)
> + * List items support the following attributes:
> + *  - file: The location of the loaded image file.
> + *  - hidpi: An alternative hidpi file location for retina screens, if one exists.
> + *           May be the magic string <not loaded> in strange cases where
> + *           only the low-resolution image is loaded but not shown.
> + *  - platforms: An array of the platforms where the issue is occurring.

You may want to document the supported platforms, as win could be windows and macosx could be mac...

Should we make the platform field optional and assume that a whitelist entry applies to all platforms when the platform isn't specified?

::: browser/base/content/test/performance/browser_startup_images.js:157
(Diff revision 1)
> +  },
> +];
> +
> +function test() {
> +  let data = Cc["@mozilla.org/test/startuprecorder;1"].getService().wrappedJSObject.data.images;
> +  let platformWhitelist = whitelist.filter((el) => el.platforms.includes(AppConstants.platform));

nit: we don't need the parens around 'el'

::: browser/base/content/test/performance/browser_startup_images.js:183
(Diff revision 1)
> +
> +  for (let item of platformWhitelist) {
> +    if (!item.intermittentNotLoaded ||
> +        !item.intermittentNotLoaded.includes(AppConstants.platform)) {
> +      if (window.devicePixelRatio >= 2 && item.hidpi) {
> +        ok(loadedImages.has(item.hidpi) || item.hidpi == "<not loaded>", `Whitelisted image ${item.hidpi} was loaded.`);

"Whitelisted image <not loaded> was loaded." will be a confusing test output.

::: browser/base/content/test/performance/browser_startup_images.js:185
(Diff revision 1)
> +    if (!item.intermittentNotLoaded ||
> +        !item.intermittentNotLoaded.includes(AppConstants.platform)) {
> +      if (window.devicePixelRatio >= 2 && item.hidpi) {
> +        ok(loadedImages.has(item.hidpi) || item.hidpi == "<not loaded>", `Whitelisted image ${item.hidpi} was loaded.`);
> +      } else {
> +        ok(loadedImages.has(item.file), `Whitelisted image ${item.file} was loaded.`);

If the point of this check is to prevent stale entries in the whitelist, should we also verify that the item was not shown?

::: browser/components/tests/startupRecorder.js:34
(Diff revision 1)
> -  this.data = {};
> +  this.data = {
> +    images: {
> +      "image-shown": new Set(),
> +      "image-loaded": new Set(),
> +    },
> +    js: { }

I don't think 'js' is a correct name here, as this will contain info about services implemented in C++.. Maybe 'code'?

nit: I usually write {} rather than { } for an empty object, but I'm not sure we have any consistent coding style on this point.
Attachment #8878561 - Flags: review?(florian) → review+
Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

https://reviewboard.mozilla.org/r/149890/#review155020

> You may want to document the supported platforms, as win could be windows and macosx could be mac...
> 
> Should we make the platform field optional and assume that a whitelist entry applies to all platforms when the platform isn't specified?

I'd rather specify the platforms explicitly, it's not so much overhead right now.
The try push is looking green, even after retriggers. I'm a bit worried that there's no way to run try for hidpi screens afaik, so I would want to open a follow-up for looking into automating this test for hidpi screens.

I think we can also have this patch resolve this bug and open follow-up bugs for the individual whitelist items.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e137dfb0bff
Add a test for images loaded at startup vs. images shown at startup. r=florian,jwatt
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=109214906&repo=autoland
Flags: needinfo?(jhofmann)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/023b96f48df8
Backed out changeset 7e137dfb0bff for failures like browser_startup_images.js | Loaded image chrome://browser/skin/sidebars.svg was shown
(In reply to Johann Hofmann [:johannh] from comment #25)
> The try push is looking green, even after retriggers. I'm a bit worried that
> there's no way to run try for hidpi screens afaik, so I would want to open a
> follow-up for looking into automating this test for hidpi screens.
> 
> I think we can also have this patch resolve this bug and open follow-up bugs
> for the individual whitelist items.

For HiDPI screens all we will need to do is set layout.css.devPixelsPerPx to 2 before starting up Firefox. You could run the test once with the default value, change the pref before shutting down, then run the test a second time using the changed value, then reset the pref and finish the test. We won't actually need HiDPI screens/hardware to simulate the environment.
Try is green, I'll try to land this. (Let's see if something changed under our feet again)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #30)
> (In reply to Johann Hofmann [:johannh] from comment #25)
> > The try push is looking green, even after retriggers. I'm a bit worried that
> > there's no way to run try for hidpi screens afaik, so I would want to open a
> > follow-up for looking into automating this test for hidpi screens.
> > 
> > I think we can also have this patch resolve this bug and open follow-up bugs
> > for the individual whitelist items.
> 
> For HiDPI screens all we will need to do is set layout.css.devPixelsPerPx to
> 2 before starting up Firefox. You could run the test once with the default
> value, change the pref before shutting down, then run the test a second time
> using the changed value, then reset the pref and finish the test. We won't
> actually need HiDPI screens/hardware to simulate the environment.

Thanks for the hint! After looking into this I'd rather open a new bug for high dpi support. It is complicated enough already right now.
Flags: needinfo?(jhofmann)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2f4da0ae681
Add a test for images loaded at startup vs. images shown at startup. r=florian,jwatt
Backed out for failing own test browser_startup_images.js, at least on Windows 7 VM debug with e10s:

https://hg.mozilla.org/integration/autoland/rev/6b0f55d8a2d24606dff719d52a3e6598ce92664e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d2f4da0ae6819be3b3ced71947f66585df58e145&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=109854752&repo=autoland

 634 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_images.js | Whitelisted image chrome://browser/skin/urlbar-history-dropmarker.png was not shown. -
638 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_images.js | Whitelisted image chrome://browser/skin/reload-stop-go.png was not shown. -
Flags: needinfo?(jhofmann)
Depends on: 1376259
Depends on: 1376379
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2cb9f9ba6b91
Add a test for images loaded at startup vs. images shown at startup. r=florian,jwatt
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3532c51a0635
Backed out changeset 2cb9f9ba6b91 on a CLOSED TREE for failures in browser_startup_images.js | Loaded image chrome://browser/skin/bookmark-hollow.svg was shown
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31f204fd0550
Add a test for images loaded at startup vs. images shown at startup. r=florian,jwatt
Flags: needinfo?(jhofmann)
https://hg.mozilla.org/mozilla-central/rev/31f204fd0550
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.2 - Jul 10
There are other ways that a RasterImage gets used beside DrawInternal. GetImageContainer and GetFrame/GetFrameAtSize specifically.
Depends on: 1377920
(In reply to Timothy Nikkel (:tnikkel) from comment #44)
> There are other ways that a RasterImage gets used beside DrawInternal.
> GetImageContainer and GetFrame/GetFrameAtSize specifically.

I should have referred the RasterImage changes to you. It seems a bit weird that the code for decoding images would be duplicated in multiple places though. Isn't there a single, central place we can hook into for raster images?

Johann, I guess you're not hitting any of these paths otherwise you'd have complained about the images not being reported correctly, but it would still be good to have this code correctly cover these bases. Once tnikkel replies to my question above can you create a follow-up patch to address his comments and get him to review (maybe in a new bug)?
(In reply to Jonathan Watt [:jwatt] from comment #45)
> (In reply to Timothy Nikkel (:tnikkel) from comment #44)
> > There are other ways that a RasterImage gets used beside DrawInternal.
> > GetImageContainer and GetFrame/GetFrameAtSize specifically.
> 
> I should have referred the RasterImage changes to you. It seems a bit weird
> that the code for decoding images would be duplicated in multiple places
> though. Isn't there a single, central place we can hook into for raster
> images?
> 
> Johann, I guess you're not hitting any of these paths otherwise you'd have
> complained about the images not being reported correctly, 

At least not in an obvious way, but there might be a false positive I'm not recognizing.

> but it would still
> be good to have this code correctly cover these bases. Once tnikkel replies
> to my question above can you create a follow-up patch to address his
> comments and get him to review (maybe in a new bug)?

Absolutely!
Summary: Investigate 13 icons that load during startup but aren't displayed → Investigate icons that load during startup but aren't displayed
Depends on: 1380377
Depends on: 1363040
Depends on: 1379359
Depends on: 1379068
Depends on: 1378525
Depends on: 1379360
Depends on: 1382248
Needinfo for comment 45. Otherwise I'll make a bug for hooking into the function mentioned in comment 44.
Flags: needinfo?(tnikkel)
Depends on: 1382588
Depends on: 1386439
Bug 1386439 handled this ni. Clearing.
Flags: needinfo?(tnikkel)
Depends on: 1390874
Depends on: 1396624
See Also: → 1704103
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: