1440305 - Consider reworking how image requests are done on the style system.

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

See bug 1439285, for an example of what annoyances this causes.

Right now image requests are stored in style structs, in nsStyleImageRequest, along with the ImageValue that created them.

Since image requests are main-thread-only we need to have special main thread hooks (FinishStyle) to resolve images, plus thread-unsafe destructors, like StyleImageRequestCleanupTask, which need to be posted as runnables.

Furthermore, due to architectural differences between Stylo and the old style system, we reuse much less these structs, which means that we do many more calls to ImageLib (again, see the test-case in bug 1439285).

A setup that would be nice for the style system would be to only store ImageValues, and then the request to be done by nsIFrame, maybe via the document loader to deduplicate them or something.

I still don't know all the details about our current setup, but given that nsIFrame::DidSetStyleContext needs to go through all the images anyway, it looks sensible to me to move the work there.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Timothy, does comment 0 look reasonable to you? Andrew told me you may be the one most familiar with the current setup :)

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Shouldn't image cache be de-duplicating actual network requests?  Or is this about de-duplicating c++ objects named Request?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Shouldn't image cache be de-duplicating actual network requests?  Or is this
> about de-duplicating c++ objects named Request?

Yes, but see bug 1439285 comment 14, that only works for HTTP images IIUC. Also, this could be considered general post-stylo cleanup, the setup for images right now is pretty weird.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

A bit more context talking about this with Xidorn on IRC:

00:10 <emilio> xidorn: really nice find with the custom properties!
00:10 <emilio> xidorn: I had completely overlooked the ImageValue::mRequests shenanigans
00:12 <xidorn> emilio: I guess we may still want some kind of specified value cache of image request... that may also be important for memory footprint. unless we can eliminate all the complexity from the style system and just hand url to imagelib, I guess...
00:16 <xidorn> emilio: anyway, I guess that hack should be fine, then... I just wanted to understand why the specified value cache doesn't work, and see if there was a better solution to that. but sounds like what you do is the easiest at present
00:18 <emilio> xidorn: right, my point was that it's probably worth managing all the complexity in the StyleImageLoader, instead of in the specified / computed values
00:19 <emilio> xidorn: that's basically my proposal in that bug
00:19 <emilio> xidorn: and you'd basically have a cache keyed on the resolved URL, or something of the sort
00:19 <emilio> xidorn: so that works for custom properties
00:19 <emilio> xidorn: it's not about eliminating caches, but about moving all the caches to a place far away from the style structs :)
00:20 <emilio> xidorn: (I'm not sure if that makes sense to you off-hand)
00:20 <xidorn> emilio: yeah that sounds like the right way to go... and we can remove the cache from ImageValue which should remove lots of complexity for handling image values
00:21 <emilio> xidorn: right, we can even remove the request itself (since it can be done in DidSetStyleContext, and we enumerate all of them there anyway). That makes the style structs not depend on the pres context, not need FinishStyle, and all the goodness
00:22 <emilio> xidorn: also non off-main-thread release of image requests...
00:22 <xidorn> emilio: I think we use prescontext and finish style for others...
00:22 <xidorn> at least counter style :)
00:23 <emilio> xidorn: ouch, counter style :(. I thought of the combined transform stuff, but I filed bug 1440384 for it
00:23 <firebot> https://bugzil.la/1440384 — NEW, [e-mail] — GenerateCombinedTransform probably doesn't want to run at the time it runs.
00:23 <emilio> xidorn: maybe we can do the same for counters
00:24 <emilio> xidorn: anyway, removing images is 90% of FinishStyle in any case :)

Comment 5

[Timothy Nikkel (:tnikkel)]

This sounds reasonable to me. Although I'm not super familiar with the current internals of the style system with respect to images.

One concern I would have is this could potentially delay the start of image loads/decodes until the frame is constructed instead of when style is resolved.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> This sounds reasonable to me. Although I'm not super familiar with the
> current internals of the style system with respect to images.
> 
> One concern I would have is this could potentially delay the start of image
> loads/decodes until the frame is constructed instead of when style is
> resolved.

Right now at least with stylo, that's the case already. It delays loads when restyling slightly, from:

  https://searchfox.org/mozilla-central/rev/88e89261ea81860f44a44ed068cf5839aea5def6/layout/base/ServoRestyleManager.cpp#899

To:

  https://searchfox.org/mozilla-central/rev/88e89261ea81860f44a44ed068cf5839aea5def6/layout/base/ServoRestyleManager.cpp#913

But that seems nothing if the first call actually goes away...

Comment 7

[Jet Villegas (inactive)]

[Triage 2018/03/23 - P3]

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Cam, you did a bunch of work related to image requests. Mind sanity-checking comment 4? I was basically waiting for you to come back to proceed :P

Comment 9

[Cameron McCormack (:heycam)]

Simplifying and centralizing the style image request object caching in some way sounds great.

One property of the old setup was that we would cache the object only as long the CSS rule that caused it to load was still around.  This is probably not so important for style sheet modifications, which aren't that common, but might be for style="" changes, to avoid constantly growing memory if there is some script switching between different background-images or something.

Would we be able to continue to do this with the new setup?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #9)
> Simplifying and centralizing the style image request object caching in some
> way sounds great.
> 
> One property of the old setup was that we would cache the object only as
> long the CSS rule that caused it to load was still around.  This is probably
> not so important for style sheet modifications, which aren't that common,
> but might be for style="" changes, to avoid constantly growing memory if
> there is some script switching between different background-images or
> something.
> 
> Would we be able to continue to do this with the new setup?

The answer to this, btw, is "not in its current form". But we should be able to use an expiration tracker to handle images going away.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

I think we're good here for now.