920630 - Improve memory usage of mozilla::image::ImageURL

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

ImageURL (added in Bug 867755) has some redundancy in it's memory usage. This should be improved.

Comment 1

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

We can eliminate ImageURL entirely now that nsIURI implementations are immutable and threadsafe.

Comment 2

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

Attachment: 0001-Bug-920630-Part-1.-Improve-image-logging-to-support-.patch, v1

Comment 3

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

Attachment: 0002-Bug-920630-Part-2.-Change-ImageCacheKey-to-use-nsIUR.patch, v1

Comment 4

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

Attachment: 0003-Bug-920630-Part-3.-Change-Image-ImageResource-and-de.patch, v1

Comment 5

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

Attachment: 0004-Bug-920630-Part-4.-Change-rest-of-imagelib-to-use-ns.patch, v1

Comment 6

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

Attachment: 0005-Bug-920630-Part-5.-Remove-image-ImageURL.h-as-unused.patch, v1

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

Comment 7

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

Attachment: 0003-Bug-920630-Part-3.-Change-Image-ImageResource-and-de.patch, v2

Fix how I used RefPtr<nsIURI> instead of nsCOMPtr<nsIURI>.

Comment 8

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

Attachment: 0002-Bug-920630-Part-2.-Change-ImageCacheKey-to-use-nsIUR.patch, v2

Avoid copying the scheme onto stack in ImageCacheKey constructor. It would be nice if we could also avoid copying the spec for the non-blob URI case. It can get quite large. Adding an nsIURI::GetSpecHash method is probably out of scope for this bug though ;).

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8962832 [details] [diff] [review]
0002-Bug-920630-Part-2.-Change-ImageCacheKey-to-use-nsIUR.patch, v2

Review of attachment 8962832 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/ImageCacheKey.cpp
@@ +112,5 @@
>    }
>  
>    // For non-blob URIs, compare the URIs.
> +  bool equals = false;
> +  mURI->Equals(aOther.mURI, &equals);

Do we need to check the return value of Equals for error?

::: image/ImageCacheKey.h
@@ +59,2 @@
>    Maybe<uint64_t> mBlobSerial;
> +  nsCString mRef;

Can we call this mBlobRef or something so it's obvious this is only used for blobs?

Comment 10

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

(In reply to Timothy Nikkel (:tnikkel) from comment #9)
> Comment on attachment 8962832 [details] [diff] [review]
> 0002-Bug-920630-Part-2.-Change-ImageCacheKey-to-use-nsIUR.patch, v2
> 
> Review of attachment 8962832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/ImageCacheKey.cpp
> @@ +112,5 @@
> >    }
> >  
> >    // For non-blob URIs, compare the URIs.
> > +  bool equals = false;
> > +  mURI->Equals(aOther.mURI, &equals);
> 
> Do we need to check the return value of Equals for error?
> 

It is probably fine, but I'll add the check in for safety's/consistency's sake.

> ::: image/ImageCacheKey.h
> @@ +59,2 @@
> >    Maybe<uint64_t> mBlobSerial;
> > +  nsCString mRef;
> 
> Can we call this mBlobRef or something so it's obvious this is only used for
> blobs?

Will do.

Comment 11

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

tries after rebasing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f645e65c5607c18402380915644a9a107831a0e6
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36717d691054c3639575924cbf09f2f80014aa41

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff2162607d61
Part 1. Improve image logging to support direct logging of URIs and images objects. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7a040f06f9
Part 2. Change ImageCacheKey to use nsIURI directly instead of ImageURL. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/7597bbcc9d66
Part 3. Change Image/ImageResource and derived classes to use nsIURI directly instead of ImageURL. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf212d04876
Part 4. Change rest of imagelib to use nsIURI directly instead of ImageURL. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2002e5279d1
Part 5. Remove image/ImageURL.h as unused. r=tnikkel

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/ff2162607d61
https://hg.mozilla.org/mozilla-central/rev/9f7a040f06f9
https://hg.mozilla.org/mozilla-central/rev/7597bbcc9d66
https://hg.mozilla.org/mozilla-central/rev/dcf212d04876
https://hg.mozilla.org/mozilla-central/rev/e2002e5279d1