Closed Bug 920630 Opened 6 years ago Closed Last year

Improve memory usage of mozilla::image::ImageURL

Categories

(Core :: ImageLib, defect)

25 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: sworkman, Assigned: aosmond)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

ImageURL (added in Bug 867755) has some redundancy in it's memory usage. This should be improved.
We can eliminate ImageURL entirely now that nsIURI implementations are immutable and threadsafe.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Fix how I used RefPtr<nsIURI> instead of nsCOMPtr<nsIURI>.
Attachment #8962817 - Attachment is obsolete: true
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 ;).
Attachment #8962816 - Attachment is obsolete: true
Attachment #8962815 - Flags: review?(tnikkel)
Attachment #8962818 - Flags: review?(tnikkel)
Attachment #8962819 - Flags: review?(tnikkel)
Attachment #8962822 - Flags: review?(tnikkel)
Attachment #8962832 - Flags: review?(tnikkel)
Blocks: 1449559
Attachment #8962815 - Flags: review?(tnikkel) → review+
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?
Attachment #8962832 - Flags: review?(tnikkel) → review+
Attachment #8962819 - Flags: review?(tnikkel) → review+
Attachment #8962822 - Flags: review?(tnikkel) → review+
Attachment #8962818 - Flags: review?(tnikkel) → review+
(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.
Pushed by aosmond@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.