517119 - Preallocate image source data buffer based on Content-Length

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

Right now we just write the bytes to an nsTArray. Removing this speeds up Tp by 4% or more.

I'm guessing that we're wasting a lot of time constantly re-allocating and re-copying the backing buffer of the TArray. Since decoders don't care how much data they decode at once, we don't need the source data to be contiguous. Thus, we should measure the performance benefit of using some sort of chunklist instead. Ideally we'd have a linked list of 4096-byte buffers (easy on the allocator), with a nice interface and a Compact() function that would shrink the last buffer when we complete the source data. Asking around if anybody knows something like that already in the codebase somewhere.

Comment 1

[Bobby Holley (:bholley)]

Attachment: patch v1

Added a patch. Looks good on try, and I tested it to make sure the hint is correct on mainstream webpages (it appears to be).

Flagging jeff for review.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 453879 [details] [diff] [review]
patch v1


> 
> //******************************************************************************
>+/* void setSizeHint(in unsigned long sizeHint); */
>+NS_IMETHODIMP imgContainer::SetSizeHint(PRUint32 sizeHint)
>+{
>+  if (sizeHint && StoringSourceData())
>+    mSourceData.SetCapacity(sizeHint);
>+  return NS_OK;
>+}

It's probably worth adding some commentary on this method vs using a chunklist.
For what it's worth WebKit has a SharedBuffer class that keeps a set of segments
that they use for this purpose. Maybe, that's worth including too.

This function might also be better called setSourceSizeHint() because it's not really
clear from the name what size is being hinted at.



>@@ -1107,16 +1108,31 @@ NS_IMETHODIMP imgRequest::OnDataAvailabl
>     if (!disposition.IsEmpty()) {
>       nsCOMPtr<nsISupportsCString> contentDisposition(do_CreateInstance("@mozilla.org/supports-cstring;1"));
>       if (contentDisposition) {
>         contentDisposition->SetData(disposition);
>         mProperties->Set("content-disposition", contentDisposition);
>       }
>     }
> 
>+    /* Use content-length as a size hint for http channels. */
>+    if (httpChannel) {
>+      nsCAutoString contentLength;
>+      rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("content-length"),
>+                                          contentLength);
>+      if (NS_SUCCEEDED(rv)) {
>+        PRInt32 len = contentLength.ToInteger(&rv);
>+        if (len > 0)
>+          sizeHint = (PRUint32) len;
>+      }
>+
>+      // Bound the size by some reasonable default
>+      sizeHint = PR_MIN(sizeHint, 20000000);
>+    }
>+
>     LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnDataAvailable", "content type", mContentType.get());
> 
>     //
>     // Figure out if our container initialization flags
>     //
> 
>     // We default to the static globals
>     PRBool isDiscardable = gDiscardable;
>@@ -1168,16 +1184,19 @@ NS_IMETHODIMP imgRequest::OnDataAvailabl
>       // is null to determine things like size availability (they should
>       // be checking the image status instead).
>       mImage = nsnull;
> 
>       this->Cancel(rv);
>       return NS_BINDING_ABORTED;
>     }
> 
>+    // Pass along the size hint. This is a no-op if sizeHint == 0.
>+    mImage->SetSizeHint(sizeHint);
>+

Why are these two hunks so far apart?

Comment 3

[Bobby Holley (:bholley)]

Attachment: patch v2

Addressed jeff's comments. Flagging jeff for re-review and vlad for sr since this touches an interface.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 454065 [details] [diff] [review]
patch v2

looks good, but bump the IID of imgIContainer.idl

Comment 5

[Bobby Holley (:bholley)]

Attachment: pushed patch

Added the final patch with the UUID rev.

Comment 6

[Bobby Holley (:bholley)]

pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/c4eefb47ed61

Resolving fixed.