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

VERIFIED FIXED in mozilla23

Status

()

Core
ImageLib
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: Alfred Kayser, Assigned: Alfred Kayser)

Tracking

({memory-footprint, perf})

unspecified
mozilla23
memory-footprint, perf
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: MemShrink)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

7 years ago
Created attachment 559088 [details] [diff] [review]
Proof of concept patch

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

Comment 2

7 years ago
Created attachment 560531 [details] [diff] [review]
Compilable and tested version
Attachment #559088 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 4

5 years ago
Created attachment 745036 [details] [diff] [review]
V2: Don't add back mAlphaBits and update to current build
Attachment #560531 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 745039 [details] [diff] [review]
V2: Don't add back mAlphaBits and update to current build
Attachment #745036 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #745039 - Attachment is patch: true
Attachment #745039 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 6

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Updated

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