Closed Bug 1299498 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: ImageLib, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: aosmond)

References

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(1 file)

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?
Flags: needinfo?(seth.bugzilla)
This might also explain some slowness I saw in creating image channels in this benchmark:

https://jakearchibald.github.io/service-worker-benchmark/
Whiteboard: [gfx-noted]
Given Seth has left and hasn't been active in bugzilla the last few months, pinging Milan to get this P1 an owner. :-)
Flags: needinfo?(seth.bugzilla) → needinfo?(milan)
P1 is an illusion :) used for scheduling.
Assignee: nobody → aosmond
Flags: needinfo?(milan)
Priority: P1 → P3
Attachment #8814887 - Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7834b7b4050
Keep a main thread only pointer to the underlying nsIURI for ImageURL. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/f7834b7b4050
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assuming 52 is also affected. Tim, is this something you might want to uplift to 52 or even 51 beta?
Flags: needinfo?(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.
Flags: needinfo?(tnikkel)
That sounds sensible, thanks.
You need to log in before you can comment on or make changes to this bug.