Closed Bug 411718 Opened 16 years ago Closed 16 years ago

Speed up JPEG decoding by 30% by skipping buffer

Categories

(Core :: Graphics: ImageLib, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta3

People

(Reporter: alfredkayser, Assigned: alfredkayser)

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file, 4 obsolete files)

Attached patch V1: Skip the read buffer (obsolete) — Splinter Review
The current JPEG decoder uses plain 'Read' in the WriteFrom into an allocated buffer. This means that all source data of the image is copied from the input stream to the buffer in the decoder from where it is decoded.
Other decoders uses ReadSegments to allow the decoder to directly process the source data from the input stream.

So, apply this trick also to the nsJPEGDecoder, and suddenly JPEG decoding is about 30% faster (before 0.09 on my jpegtest page, and 0.064 after, so 30% difference!) (and it saves a buffer allocation).

Note, the 'SetMutable' doesn't have any effect, as the imgContainer always optimizes the first frame, so the mImageLoad member can also be removed.

P.s. it seems that the BackBuffer is very rarely bigger than 256 bytes, but generally used for each image where source data > 4K, so this could be allocated as part of the decoder class using an AutoArray or such?
Attachment #296373 - Flags: review?(pavlov)
Attachment #296373 - Flags: approval1.9?
Version: unspecified → Trunk
I noticed we weren't using this a few weeks ago.  Surprised it is 30% but will be good to get in.
Comment on attachment 296373 [details] [diff] [review]
V1: Skip the read buffer

Needs review before requesting approval.
Attachment #296373 - Flags: approval1.9?
Moving to blocking list so this can land once we get the right reviews + impl..
Flags: blocking1.9+
Priority: -- → P2
Comment on attachment 296373 [details] [diff] [review]
V1: Skip the read buffer

this looks good.
Attachment #296373 - Flags: review?(pavlov) → review+
Attachment #296373 - Flags: superreview?(tor)
Attachment #296373 - Flags: approval1.9?
Comment on attachment 296373 [details] [diff] [review]
V1: Skip the read buffer

You are blocking already - so free ot land once you have the right reviews
Attachment #296373 - Flags: approval1.9?
Comment on attachment 296373 [details] [diff] [review]
V1: Skip the read buffer

>- /* Check for malformed MARKER segment lengths. */
>-    if (new_backtrack_buflen > MAX_JPEG_MARKER_LENGTH) {
>+printf("BackBuffer %d\n", roundup_buflen);
>+    JOCTET *buf = (JOCTET *)PR_REALLOC(decoder->mBackBuffer, roundup_buflen);
>+    /* Check for OOM */
>+    if (!buf) {
>+      decoder->mInfo.err->msg_code = JERR_OUT_OF_MEMORY;

The debugging printf should be removed before checkin.
Attachment #296373 - Flags: superreview?(tor) → superreview+
Keywords: checkin-needed
Attachment #296373 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Checking in modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp,v  <--  nsJPEGDecoder.cpp
new revision: 1.83; previous revision: 1.82
done
Checking in modules/libpr0n/decoders/jpeg/nsJPEGDecoder.h;
/cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.h,v  <--  nsJPEGDecoder.h
new revision: 1.31; previous revision: 1.30
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
After the check-in, MH jumped up significantly everywhere 
and tp and tdhtml on bm-xserve08 don't look good either.
That cannot be caused by this patch. The jump happened before the code was compiled:
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1200736140

The checkins that happened before the jump are:
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1200727560&maxdate=1200731279
Especially the backout of https://bugzilla.mozilla.org/show_bug.cgi?id=412340 may be the cause.
The Tp jump on bm-xserve08 certainly happened before this landed, but not the MH jumps.
See the links in comment #10, the MH is clearly visible when the code still hasn't compiled yet.
And the patch actually removes a 'malloc' from the decoder (so that now only one (re)alloc is left for the allocation of the backbuffer, which is never more than 256 bytes anyway).
I think the Tp regression is the JS patch, but the MH regression is definitely this patch. smaug backed it out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Before the backout:
Leaks: 3799217 bytes, 29100 allocations
Maximum Heap Size: 19423723 bytes
105192543 bytes were allocated in 516343 allocations.
Logged 4936 free (or realloc) calls for which we missed the original malloc.

After the Backout:
Leaks: 3799348 bytes, 29105 allocations
Maximum Heap Size: 18732033 bytes
105058163 bytes were allocated in 515818 allocations.
Logged 4881 free (or realloc) calls for which we missed the original malloc.

Possibly there is something with the accounting of realloc(null, size)?


So, this patch removes that realloc optimization. 
Can this be submitted again, to see if this helps?
Attachment #297580 - Attachment is obsolete: true
(In reply to comment #18)
> 
> So, this patch removes that realloc optimization. 
> Can this be submitted again, to see if this helps?

Sounds reasonable to me.

(In reply to comment #19)
> (In reply to comment #18)
> > 
> > So, this patch removes that realloc optimization. 
> > Can this be submitted again, to see if this helps?
> 
> Sounds reasonable to me.
> 

Yes - please do.   We want this patch for sure.
(In reply to comment #18)
> Created an attachment (id=298093) [details]
> V3: Skip the 'realloc' optimization
> 
> Before the backout:
> Leaks: 3799217 bytes, 29100 allocations
> Maximum Heap Size: 19423723 bytes
> 105192543 bytes were allocated in 516343 allocations.
> Logged 4936 free (or realloc) calls for which we missed the original malloc.
> 
> After the Backout:
> Leaks: 3799348 bytes, 29105 allocations
> Maximum Heap Size: 18732033 bytes
> 105058163 bytes were allocated in 515818 allocations.
> Logged 4881 free (or realloc) calls for which we missed the original malloc.
> 
> Possibly there is something with the accounting of realloc(null, size)?
> 
> 
> So, this patch removes that realloc optimization. 
> Can this be submitted again, to see if this helps?
> 

Bug 334285 related?
I don't have access to bug 334285...
Landed v3.

Checking in modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp,v  <--  nsJPEGDecoder.cpp
new revision: 1.85; previous revision: 1.84
done
Checking in modules/libpr0n/decoders/jpeg/nsJPEGDecoder.h;
/cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.h,v  <--  nsJPEGDecoder.h
new revision: 1.33; previous revision: 1.32
done
Still getting an MH regression with v3. backed out.
Found the issue:
With this SetMutable() was removed (assuming that DecodingComplete did it already), however DecodingComplete is never called by nsJPEGDecoder.
So, the image is now not 'optimized' anymore, keeping the complete image around (explaining the jump of .6MB of mH)

This patch doesn't include the mLoadImage->SetMutable removal part and will thus not cause a mH jump.
Attachment #298093 - Attachment is obsolete: true
Can this be checked in, as now the cause of the leak has been detected and removed?
The normal procedure is to request reviews and then just check it in / add checkin-needed. Dunno if the difference between V3 and V4 is trivial enough to go without reviews. Usually it's not.
Comment on attachment 298240 [details] [diff] [review]
V4: Bring back the 'SetMutable' to prevent the mH jump

Re-requesting reviews for this patch, that is equal compared to V1, except for removal of selected:
1. Removed the printf (V1->v2)
2. Removed the 'malloc/realloc' optimization of mBackBuffer (v2->v3)
3. Removed the 'mImageLoad' optimization as it caused the mH jump, because then images weren't optimized anymore (v3->v4).

The diff's between these version clearly show these differences. 

The real part is the ReadSegment which is untouched since version 1.

But, after two backouts better to be safe and do the reviews again, I assume.
Attachment #298240 - Flags: superreview?(tor)
Attachment #298240 - Flags: review?(pavlov)
(In reply to comment #28)
> 2. Removed the 'malloc/realloc' optimization of mBackBuffer (v2->v3)

Could we try leaving the optimization in since it wasn't the cause of the MH problem?
(In reply to comment #29)
> (In reply to comment #28)
> > 2. Removed the 'malloc/realloc' optimization of mBackBuffer (v2->v3)
> 
> Could we try leaving the optimization in since it wasn't the cause of the MH
> problem?
> 

Yea - agreed.  Pav can you help get this reviewed asap so we can close this out?
can we get a patch up with that not removed?
Attachment #298240 - Attachment is obsolete: true
Attachment #298413 - Flags: superreview?(tor)
Attachment #298413 - Flags: review?(pavlov)
Attachment #298240 - Flags: superreview?(tor)
Attachment #298240 - Flags: review?(pavlov)
Attachment #298413 - Flags: superreview?(tor) → superreview+
Keywords: checkin-needed
Attachment #298413 - Flags: review?(pavlov) → review+
Checking in modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp,v  <--  nsJPEGDecoder.cpp
new revision: 1.87; previous revision: 1.86
done
Checking in modules/libpr0n/decoders/jpeg/nsJPEGDecoder.h;
/cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.h,v  <--  nsJPEGDecoder.h
new revision: 1.35; previous revision: 1.34
done
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.