1027106 - Make ImageLib share cached imgRequests for the same SVG document even if the URI has a different ref/hash

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Jonathan Watt [:jwatt]]

In order to get anywhere close to fixing bug 999931 we'll need content authors to put their icons into a single "icon set" SVG document, and have them reference individual icons using a fragment identifier (e.g. in <img> element's 'src' attributes). Even with the latest and potential future improvements, the overhead in creating individual SVG documents for each icon is simply too much for memory limited Firefox OS devices.

The one thing currently blocking "icon set" SVG documents is that we currently do not share imgRequest objects when URIs that are requested have different ref/hash parts in their URI spec. In other words we load a separate SVGDocument for each imgRequest with a different ref/hash. We need to fix that if an "icon set" SVG documents is to perform any better than having each icon in its own SVG document.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: WIP

This is just to give an idea of what I'm talking about. Obviously there's a bunch of work to do with surface caching and other things so that this doesn't all go wrong.

Comment 2

[Jonathan Watt [:jwatt]]

I'd note that ImageFactory::CreateRasterImage does seem to care about the ref, since it seems to have code to implement:

http://www.w3.org/TR/media-frags/

Not sure at this point if that means we can't always ignore the fragment identifier for caching purposes. If so, maybe we can restrict ignoring the fragment identifier to vector images?

Comment 3

[Seth Fowler [:seth] [:s2h]]

So I am actually working on patches related to this issue now. (Although my motivation is a bit different.)

As things stand right now (where we can only have a single decoded version of an image in each RasterImage instance), sharing the cache entries for the same URI with different refs is fairly nontrivial to support without breaking "-moz-resolution". My focus right now is on creating patches that will let us store multiple decoded images in a single RasterImage instance, which will let us replace "-moz-resolution" with a better solution. (Or at least reimplement it in a cleaner way.)

This exact issue has also prevented us from landing support for any of the other image-related media fragments, which need the same cache behavior you need for SVG documents. That's one of the reasons I'm working on fixing this problem.

An additional consideration is that Honza and Steve from the networking team are planning to add some new features to the Necko cache soon that will allow us to completely remove the imagelib cache. That's part of why I haven't implemented any kind of workaround: this code is all going away later this year.

In some sense the patches I'm working on are the long-term, correct solution to the problem. However, they're larger in scope what you need and will take some time to start landing. If you want to get this fixed as quickly as possible, before those patches are ready, I think your best bet is to solve this problem without involving imgLoader at all.

I would instead add a shared SVG document cache to imagelib, keyed on the URI spec (not including the ref). It needs to have different semantics to the surface cache, in that (for now) we won't expire things out of it just because they haven't been touched in a long time. Instead, each VectorImage that shares a given document will increment a reference count on that document. When the VectorImages get destroyed and the reference count his zero, the cache entry will get destroyed as well.

There may be some concurrency involved here as well, since we deliver OnDataAvailable off-main-thread, and that needs to touch the cache entries. But it shouldn't be too complex.

We will have other uses for a cache like this going forward.

By the way, you cannot distinguish between raster and vector images in imgLoader. We can't tell for sure until we have data. Both the mime type reported by the server and the actual content that gets sent down the wire are factors. I strongly recommend that you avoid trying to implement this in imgLoader.

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 8442133 [details] [diff] [review]
WIP

Thanks for going into such detail, Seth. That's all very helpful. :)

Comment 5

[Seth Fowler [:seth] [:s2h]]

Jonathan, I talked to dbaron about this a bit and he mentioned that we have some existing caching similar to what we'd need here: nsExternalResourceMap, which is part of nsDocument. It's used for SVG external resource documents, in a very similar way to what we need here - it merges different document URIs which only differ by their ref.

We can't rely on nsDocument's existing cache since it's per-document, but we may still be able to learn from the implementation.

Comment 6

[:Paolo Amadini]

Referencing the same SVG with different hash parts is a technique we use a lot in the Desktop front-end, here is a rough search for this pattern:

http://mxr.mozilla.org/mozilla-central/search?string=.svg%23&find=browser

I've just learned that each distinct hash reference creates an entire new document object in bug 1054016 comment 88. This is particularly bad because it makes the increase in memory usage and set up time _quadratic_ when adding new glyphs to an SVG file.

And if we use a separate file for each new glyph, we get the possibly worse I/O overhead of loading each file from disk separately.

This applies with either "icon stacks", that use styles to isolate the target, or "sprites" where the target area is identified by a "symbol" element or a "viewBox" entry in the hash part of the URL. The only workaround is to use "-moz-image-region" and "-moz-image-rect" with one SVG file, sacrificing legibility.

I've spent a lot of time making sense of and debugging image regions when working on front-end features like the Downloads Panel, especially because different platforms often end up using different image layouts, and HiDPI always uses a different coordinate system.

Fixing this bug will probably result in a memory usage reduction and load time improvement right now, even before we start using SVG more widely as we plan to do in the Firefox front-end in the near future.

Comment 7

[Mark Turner]

This could be why I see full page svg aggressively cached?
I am serving up entire xml/svg pages with changes.
Though I've been mystified as to why FF is caching them.
For me this is not the expected result.
Is there a possibility of avoiding this with a header?
Etag/cache or some such?

Comment 9

[hagenauer.florian]

Any progress on this issue, especially that discussion in #1121693 stopped.
This would be really helpful for the performance of combined SVG icons.

Comment 11

[zdnex]

hi, old issue about this problem were closed so I am wondering if there is planed fix for this?

Comment 12

[Playest]

What is the current status of this bug? I noticed it today when I was trying to optimize load time of my website so I can confirm it's still active.

Comment 13

[sally]

I'm unable to reproduce this problem in neither Firefox for Linux (75) nor Mac (76.0.01)
https://justafish.github.io/sprite-test/

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to sally from comment #13)

I'm unable to reproduce this problem in neither Firefox for Linux (75) nor Mac (76.0.01)
https://justafish.github.io/sprite-test/

That test cases doesn't use images in the sense of this bug: <img>, svg <image>, background images is what this bug is talking about. That test cases uses <use>.

Comment 15

[Shubham Jain]

Any updates on this bug? This has been pending for a while, and it's a needed optimization. Both, Chrome and Safari handle this well, so only Firefox is the remaining browser.

Comment 16

[Manuel Strehl]

If you need a work-around until this bug is fixed: I just found out, that Firefox only makes one request, if you use an SVG <use> element instead of an <img> or CSS background-image. That is, replace this HTML:

<img src="sprite.svg#foo">

with this embedded SVG:

<svg><use xlink:href="sprite.svg#foo"/></svg>

(Note the closing “/>” on the <use>!)

Comment 17

[Rob]

(In reply to Manuel Strehl from comment #16)

If you need a work-around until this bug is fixed: I just found out, that Firefox only makes one request, if you use an SVG <use> element instead of an <img> or CSS background-image. That is, replace this HTML:

<img src="sprite.svg#foo">

with this embedded SVG:

<svg><use xlink:href="sprite.svg#foo"/></svg>

(Note the closing “/>” on the <use>!)

Did you notice with this technique if you reload the page the icons seem to blink as outlined here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1027106

This doesn't happen in chrome

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to Rob from comment #17)

Did you notice with this technique if you reload the page the icons seem to blink as outlined here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1027106

Did you paste the wrong link? Can you file a bug for this and cc me please?

Comment 19

[Rob]

Sorry yes I did post the wrong link
https://bugzilla.mozilla.org/show_bug.cgi?id=1678956

Comment 20

[putzeimer]

Attachment: index.htm

Here's a test case. Firefox requests each image three times, other browsers only once.
Hope this helps to fasten things up.

Comment 21

[Malolan B]

(In reply to Manuel Strehl from comment #16)

If you need a work-around until this bug is fixed: I just found out, that Firefox only makes one request, if you use an SVG <use> element instead of an <img> or CSS background-image. That is, replace this HTML:

<img src="sprite.svg#foo">

with this embedded SVG:

<svg><use xlink:href="sprite.svg#foo"/></svg>

(Note the closing “/>” on the <use>!)

Can this be used for cross-origin urls?

Comment 23

[i.vinnykov]

This bug is unresolved for 9 years, it leads to a memory problems as reported at https://bugzilla.mozilla.org/show_bug.cgi?id=1842382.

Comment 24

[i.vinnykov]

For sure, it's a performance problem. A standalone test case is attached to https://bugzilla.mozilla.org/show_bug.cgi?id=1842382.
For this test case:
Firefox: page is opened in 30-40 sec, RAM usage is 6 GB
Chrome: page is opened in 2-3 sec, RAM usage is 0.2 GB

Comment 25

[Benjamin De Kosnik [:bdekoz]]

Hey this is impacting a high-priority webperf issue, https://bugzilla.mozilla.org/show_bug.cgi?id=1842382. Any chance you can comment on status or WIP? thanks.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The Performance Impact Calculator has determined this bug's performance impact to be medium. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: [x] Windows [x] macOS [x] Linux
Page load impact: Some
Websites affected: Rare
Resource impact: Some
[x] Able to reproduce locally
[x] Multiple reporters