If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Cache IOService and imgILoader in nsImageLoadingContent

RESOLVED FIXED

Status

()

Core
Layout: Misc Code
--
minor
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

({perf})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

10.54 KB, patch
(not reading, please use seth@sspitzer.org instead)
: approval1.4b+
Details | Diff | Splinter Review
Currently, ImageLoadingContent gets the imgILoader each time it loads an image.
it would be better to cache it in a class static and release it when the module
is unloaded.


similar for nsIIOService.
See http://lxr.mozilla.org/seamonkey/source/layout/build/nsLayoutModule.cpp#270
Created attachment 122220 [details] [diff] [review]
patch

bz, thanks for the pointer
Comment on attachment 122220 [details] [diff] [review]
patch

images still load fine

mozilla also works fine if imglib is not present
Attachment #122220 - Attachment description: aptch → patch
Attachment #122220 - Flags: review?(bz-bugspam)
Comment on attachment 122220 [details] [diff] [review]
patch

Excellent.

I'd actually ask for approval, as this may help pageload some....
Attachment #122220 - Flags: superreview+
Attachment #122220 - Flags: review?(bz-bugspam)
Attachment #122220 - Flags: review+
Comment on attachment 122220 [details] [diff] [review]
patch

this patch just caches two services that would otherwise be gotten on each
image load.

as this will probably improve Tp a bit, it would be nice to have it in 1.4b.
Attachment #122220 - Flags: approval1.4b?
bz / christian

are you sure this is a perf win?

the reason I ask is that under the covers, getService() does some caching.

I think repeating callings are just going to be lookups in a hash table.

what's the risk-to-reward for this?

Comment 7

15 years ago
   // Get the image loader...
-  nsresult rv;
-  nsCOMPtr<imgILoader> loader = do_GetService("@mozilla.org/image/loader;1", &rv);

Remove the comment ;-)

Seth: you're asking whether calling a function which then calls a function to
do a hash lookup could be compared to accessing a static variable?

Yes hashtable lookups are relatively cheap when compared to constructing objects
but two function calls are much more expensive than the cost of a single static
variable.
There's basically no risk, Seth.  Static var access is _much_ faster than
hashing the contractid string every time...

I'm still looking into what may have caused a slowdown when I landed loading
images from content.... and one of the things image frames did was cache the
IOService.
Comment on attachment 122220 [details] [diff] [review]
patch

timless, the claim was that this fix will improve Tp.

I'm just pointing out that it may not make a difference.

does someone have some actual Tp numbers, or would we need to land and	check
tinderbox?

based on bz's comment, a=sspitzer

but let's keep an eye out for possible shutdown problems.

also, once this lands, can someone report back with the Tp improvements?

that will help me a= faster next time I see something like this.
Attachment #122220 - Flags: approval1.4b? → approval1.4b+
checked in; I do not (yet) have actual Tp numbers, partly because Tp tests can
only be run from inside the firewall

I will watch tinderbox and paste Tp numbers here when I have them.

Even if this does not help Tp much, it shouldn't hurt it, so I believe that this
patch was worth getting checked in.

marking fixed.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
No effect on Linux btek and luna

~10 ms (~1.65%) Tp improvement on monkey which surprises me, not even the other
OSX box (silverstone) shows a change.

No change on WINNT Creature.
ah. the monkey improvement looks like it was just some noise
You need to log in before you can comment on or make changes to this bug.