Closed Bug 685471 Opened 9 years ago Closed 8 years ago

In PNG decoder, the allocating of mHeaderBuf can be prevented.

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla23

People

(Reporter: alfredkayser, Assigned: alfredkayser)

Details

(Keywords: memory-footprint, perf, Whiteboard: MemShrink)

Attachments

(1 file, 3 obsolete files)

mHeaderBuf is dynamically allocated when PNG images are scanned for size only (isSizeDecode), but it is only 24 bytes, so replacing a 4 bytes pointer with a 24 bytes array in the PNGdecoder class is already better than mallocing 24 bytes (causing fragmentation), but actually only an 4 bytes buffer buffer is needed to get the width and height.
Status: NEW → ASSIGNED
Attached patch Proof of concept patch (obsolete) — Splinter Review
Remove the need to allocate a dynamic buffer by only buffering the size bytes into an 8 bytes buffer. 

By also sorting the members of the class, the total class size remains the same:
before: PRUint8 *, PRUint32, PRUint8, PRPackedBool, PRPackedBool, PRUint32, PRPackedBool = 20 bytes
after: PRUint8[8], PRUint32, PRUint32, PRUint8, PRPackedBool, PRPackedBool, PRPackedBool = 20 bytes
Attached patch Compilable and tested version (obsolete) — Splinter Review
Attachment #559088 - Attachment is obsolete: true
Keywords: footprint
I don't see why this patch adds "PRUint8 mAlphaBits;"
to the header file.  That might be part of the patch for a different bug.
Attachment #560531 - Attachment is obsolete: true
Attachment #745036 - Attachment is obsolete: true
Attachment #745039 - Attachment is patch: true
Attachment #745039 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 745039 [details] [diff] [review]
V2: Don't add back mAlphaBits and update to current build

Green tryserver run (except for the usual intermittent issues):
https://tbpl.mozilla.org/?tree=Try&rev=beec8c74c875
Attachment #745039 - Flags: review?(glennrp+bmo)
Comment on attachment 745039 [details] [diff] [review]
V2: Don't add back mAlphaBits and update to current build

Based on my code examination and your try-server results, +r.  Not sure I'm a reviewer, so Joe, please review also.
Attachment #745039 - Flags: review?(joe)
Attachment #745039 - Flags: review?(glennrp+bmo)
Attachment #745039 - Flags: review+
Comment on attachment 745039 [details] [diff] [review]
V2: Don't add back mAlphaBits and update to current build

Review of attachment 745039 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.

::: image/decoders/nsPNGDecoder.cpp
@@ +110,5 @@
>   : Decoder(aImage),
>     mPNG(nullptr), mInfo(nullptr),
>     mCMSLine(nullptr), interlacebuf(nullptr),
>     mInProfile(nullptr), mTransform(nullptr),
> +   mHeaderBytesRead(0), mCMSMode(0), 

please remove the trailing whitespace

@@ +305,5 @@
> +        mSizeBytes[mHeaderBytesRead - WIDTH_OFFSET] = bptr[pos];
> +      }
> +      pos ++;
> +      mHeaderBytesRead ++;
> +    } 

trailing whitespace all over here
Attachment #745039 - Flags: review?(joe) → review+
Please don't add PR integer types. We haven't used them since last August.
https://hg.mozilla.org/mozilla-central/rev/cf288039ea21
https://hg.mozilla.org/mozilla-central/rev/04abf5e608f1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.