Closed
Bug 411718
Opened 17 years ago
Closed 17 years ago
Speed up JPEG decoding by 30% by skipping buffer
Categories
(Core :: Graphics: ImageLib, defect, P2)
Core
Graphics: ImageLib
Tracking
()
VERIFIED
FIXED
mozilla1.9beta3
People
(Reporter: alfredkayser, Assigned: alfredkayser)
Details
(Keywords: memory-footprint, perf)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
pavlov
:
review+
tor
:
superreview+
|
Details | Diff | 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?
Updated•17 years ago
|
Version: unspecified → Trunk
Comment 1•17 years ago
|
||
I noticed we weren't using this a few weeks ago. Surprised it is 30% but will be good to get in.
Comment 2•17 years ago
|
||
Comment on attachment 296373 [details] [diff] [review]
V1: Skip the read buffer
Needs review before requesting approval.
Attachment #296373 -
Flags: approval1.9?
Comment 3•17 years ago
|
||
Moving to blocking list so this can land once we get the right reviews + impl..
Flags: blocking1.9+
Priority: -- → P2
Comment 4•17 years ago
|
||
Comment on attachment 296373 [details] [diff] [review]
V1: Skip the read buffer
this looks good.
Attachment #296373 -
Flags: review?(pavlov) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #296373 -
Flags: superreview?(tor)
Assignee | ||
Updated•17 years ago
|
Attachment #296373 -
Flags: approval1.9?
Comment 5•17 years ago
|
||
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+
Updated•17 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•17 years ago
|
||
Attachment #296373 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Comment 8•17 years ago
|
||
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: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Comment 9•17 years ago
|
||
After the check-in, MH jumped up significantly everywhere
and tp and tdhtml on bm-xserve08 don't look good either.
Assignee | ||
Comment 10•17 years ago
|
||
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.
Comment 11•17 years ago
|
||
The Tp jump on bm-xserve08 certainly happened before this landed, but not the MH jumps.
Assignee | ||
Comment 12•17 years ago
|
||
See the links in comment #10, the MH is clearly visible when the code still hasn't compiled yet.
Assignee | ||
Comment 13•17 years ago
|
||
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).
Comment 14•17 years ago
|
||
On fxdbug-linux-tbox the MH regression is right after this patch landed
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1200732300&maxdate=1200733739
Comment 15•17 years ago
|
||
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 → ---
Comment 16•17 years ago
|
||
After backing out MH came back to normal level.
http://build-graphs.mozilla.org/graph/query.cgi?tbox=fxdbug-linux-tbox.build.mozilla.org&testname=trace_malloc_maxheap&autoscale=1&size=&units=bytes<ype=&points=&showpoint=2008%253A01%253A19%253A15%253A21%253A50%252C22019097&avg=1&days=2
and seamonkey:
http://ajschult.homelinux.org:8080/graph/query.cgi?tbox=nye_bloat&testname=trace_malloc_maxheap&autoscale=1&size=&units=bytes<ype=&points=&showpoint=2008%253A01%253A19%253A15%253A12%253A01%252C18732033&avg=1&days=2
Comment 17•17 years ago
|
||
(In reply to comment #16)
> After backing out MH came back to normal level.
> http://build-graphs.mozilla.org/graph/query.cgi?tbox=fxdbug-linux-tbox.build.mozilla.org&testname=trace_malloc_maxheap&autoscale=1&size=&units=bytes<ype=&points=&showpoint=2008%253A01%253A19%253A15%253A21%253A50%252C22019097&avg=1&days=2
>
> and seamonkey:
> http://ajschult.homelinux.org:8080/graph/query.cgi?tbox=nye_bloat&testname=trace_malloc_maxheap&autoscale=1&size=&units=bytes<ype=&points=&showpoint=2008%253A01%253A19%253A15%253A12%253A01%252C18732033&avg=1&days=2
>
This is implicated by the private bytes test as well:
http://graphs.mozilla.org/#spst=range&spss=1200704253.1839495&spse=1200764031.223625&spstart=1196881975&spend=1200781613&bpst=Cursor&bpstart=1200704253.1839495&bpend=1200764031.223625&m1tid=53222&m1bl=0&m1avg=0&m2tid=53258&m2bl=0&m2avg=0&m3tid=53240&m3bl=0&m3avg=0&m4tid=53280&m4bl=0&m4avg=0
Which is a bummer because this definitely speeds up JPEG decoding.
Any ideas Michael or Pav - are we forgetting to free memory, have a dangling reference or something?
Assignee | ||
Comment 18•17 years ago
|
||
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
Comment 19•17 years ago
|
||
(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.
Comment 20•17 years ago
|
||
(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.
Comment 21•17 years ago
|
||
(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?
Assignee | ||
Comment 22•17 years ago
|
||
I don't have access to bug 334285...
Comment 23•17 years ago
|
||
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
Comment 24•17 years ago
|
||
Still getting an MH regression with v3. backed out.
Assignee | ||
Comment 25•17 years ago
|
||
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
Assignee | ||
Comment 26•17 years ago
|
||
Can this be checked in, as now the cause of the leak has been detected and removed?
Comment 27•17 years ago
|
||
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.
Assignee | ||
Comment 28•17 years ago
|
||
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)
Comment 29•17 years ago
|
||
(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?
Comment 30•17 years ago
|
||
(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?
Comment 31•17 years ago
|
||
can we get a patch up with that not removed?
Assignee | ||
Comment 32•17 years ago
|
||
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+
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Updated•17 years ago
|
Attachment #298413 -
Flags: review?(pavlov) → review+
Comment 33•17 years ago
|
||
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: 17 years ago → 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•