1299498 - imgIRequest::GetURI is now very slow, which is a bad idea for restyling performance

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

This is fallout from bug 867755.  In that bug, the contract for imgIRequest::GetURI silently changed from "just do an addref" to "create a whole new nsIURI", which is quite a bit slower.  Unfortunately, GetURI is called with callstacks like this:

#3      0x0000000103c8d897 in imgRequestProxy::GetURI(nsIURI**) at image/imgRequestProxy.cpp:540
#4      0x000000010545b3cf in EqualImages(imgIRequest*, imgIRequest*) at layout/style/nsStyleStruct.cpp:88
#5      0x000000010545f626 in nsStyleImage::operator==(nsStyleImage const&) const at layout/style/nsStyleStruct.cpp:2300
#6      0x000000010545faac in nsStyleImage::operator!=(nsStyleImage const&) const [inlined] at layout/style/nsStyleStruct.h:434
#7      0x000000010545faa1 in nsStyleImageLayers::Layer::CalcDifference(nsStyleImageLayers::Layer const&, nsChangeHint) const at layout/style/nsStyleStruct.cpp:2769

which are rather performance-sensitive.

Presumably what we need is an API on imgIRequest to check whether it has the same URI as another imgIRequest, which it could then implement however it sees fit...

Note that nsStandardURL::Equals does NOT just do string compares (e.g. for file:// URIs things are complicated), but maybe for the style system use case that's ok...

Or maybe ImageURL should cache the nsIURI it's initialized from?  It's not clear to me where these objects can be destroyed.

Seth, any suggestions for who should implement this?

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This might also explain some slowness I saw in creating image channels in this benchmark:

https://jakearchibald.github.io/service-worker-benchmark/

Comment 2

[:Gijs (out for now; he/him)]

Given Seth has left and hasn't been active in bugzilla the last few months, pinging Milan to get this P1 an owner. :-)

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

P1 is an illusion :) used for scheduling.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Hold main-thread only pointer to original nsIURI, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eae412e5b5cee7c8d046c449774f0bf4d549ce17

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7834b7b4050
Keep a main thread only pointer to the underlying nsIURI for ImageURL. r=tnikkel

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f7834b7b4050

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Assuming 52 is also affected. Tim, is this something you might want to uplift to 52 or even 51 beta?

Comment 8

[Timothy Nikkel (:tnikkel)]

We've had problems with big URIs causing OOMs before and this patch could potentially keep around another copy of the uri so I'd prefer not to uplift this.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

That sounds sensible, thanks.