Closed Bug 517119 Opened 11 years ago Closed 10 years ago

Preallocate image source data buffer based on Content-Length

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bholley, Assigned: bholley)

Details

Attachments

(2 files, 1 obsolete file)

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.
Summary: Investigate speeding up image source data storage → Preallocate image source data buffer based on Content-Length
Attached patch patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → bobbyholley+bmo
Status: NEW → ASSIGNED
Attachment #453879 - Flags: review?(jmuizelaar)
Attachment #453879 - Attachment is patch: true
Attachment #453879 - Attachment mime type: application/octet-stream → text/plain
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?
Attachment #453879 - Flags: review?(jmuizelaar) → review-
Attached patch patch v2Splinter Review
Addressed jeff's comments. Flagging jeff for re-review and vlad for sr since this touches an interface.
Attachment #453879 - Attachment is obsolete: true
Attachment #454065 - Flags: superreview?(vladimir)
Attachment #454065 - Flags: review?
Comment on attachment 454065 [details] [diff] [review]
patch v2

looks good, but bump the IID of imgIContainer.idl
Attachment #454065 - Flags: superreview?(vladimir)
Attachment #454065 - Flags: superreview+
Attachment #454065 - Flags: review?(jmuizelaar)
Attachment #454065 - Flags: review?
Attachment #454065 - Flags: review?(jmuizelaar) → review+
Attached patch pushed patchSplinter Review
Added the final patch with the UUID rev.
pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/c4eefb47ed61

Resolving fixed.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.