Speed up JPEG decoding by 30% by skipping buffer

VERIFIED FIXED in mozilla1.9beta3

Status

()

Core
ImageLib
P2
normal
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Alfred Kayser, Assigned: Alfred Kayser)

Tracking

({footprint, perf})

Trunk
mozilla1.9beta3
footprint, perf
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 296373 [details] [diff] [review]
V1: Skip the read buffer

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

Comment 1

9 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 on attachment 296373 [details] [diff] [review]
V1: Skip the read buffer

Needs review before requesting approval.
Attachment #296373 - Flags: approval1.9?

Comment 3

9 years ago
Moving to blocking list so this can land once we get the right reviews + impl..
Flags: blocking1.9+
Priority: -- → P2

Comment 4

9 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

9 years ago
Attachment #296373 - Flags: superreview?(tor)
(Assignee)

Updated

9 years ago
Attachment #296373 - Flags: approval1.9?

Comment 5

9 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 6

9 years ago
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
(Assignee)

Comment 7

9 years ago
Created attachment 297580 [details] [diff] [review]
V2: Remove the printf, ready for checkin
Attachment #296373 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
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
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11

Comment 9

9 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

9 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

9 years ago
The Tp jump on bm-xserve08 certainly happened before this landed, but not the MH jumps.
(Assignee)

Comment 12

9 years ago
See the links in comment #10, the MH is clearly visible when the code still hasn't compiled yet.
(Assignee)

Comment 13

9 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).
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
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 → ---
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&ltype=&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&ltype=&points=&showpoint=2008%253A01%253A19%253A15%253A12%253A01%252C18732033&avg=1&days=2

Comment 17

9 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&ltype=&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&ltype=&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

9 years ago
Created attachment 298093 [details] [diff] [review]
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?
Attachment #297580 - Attachment is obsolete: true

Comment 19

9 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

9 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

9 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

9 years ago
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.
(Assignee)

Comment 25

9 years ago
Created attachment 298240 [details] [diff] [review]
V4: Bring back the 'SetMutable' to prevent the mH jump

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

9 years ago
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.
(Assignee)

Comment 28

9 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)
(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

9 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

9 years ago
can we get a patch up with that not removed?
(Assignee)

Comment 32

9 years ago
Created attachment 298413 [details] [diff] [review]
V5: With the backbuffer realloc again
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)

Updated

9 years ago
Attachment #298413 - Flags: superreview?(tor) → superreview+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed

Updated

9 years ago
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
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.