Closed Bug 1015785 Opened 6 years ago Closed 6 years ago

Fix Moz2D's AlignedArray::Realloc() to not stupidly over allocate

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox32 --- wontfix
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 1 obsolete file)

The code in AlignedArray::Realloc() is confused. Specifically the line:

  mStorage = new (std::nothrow) T[aSize + (alignment - 1)];

is trying to allocate enough memory to hold aSize Ts plus "alignment minus one" bytes. What it's actually doing is allocating aSize Ts plus "alignment minus one" Ts. This wastes a bit of memory.

Just as well Ts can't be smaller than one byte. ;)
Attached patch patch (obsolete) — Splinter Review
I've also renamed "aSize" to "aCount" to avoid the potential for people to think that it's a byte size rather than a number of elements that the functions expect.
Attachment #8428430 - Flags: review?(bas)
Comment on attachment 8428430 [details] [diff] [review]
patch

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

I like the rename.

::: gfx/2d/Tools.h
@@ +120,4 @@
>    {
>      delete [] mStorage;
> +    mStorage = reinterpret_cast<T*>(new (std::nothrow)
> +                 uint8_t[aCount * sizeof(T) + (alignment - 1)]);

This will bypass the constructors for type T and will therefor not generate the desired behavior. Having thought about this, our blunt movement of the pointer screws up initialization completely for more complex types anyway!

We need to do something with a placement new in addition to the code you put up above, i.e. the code below should become:

mPtr = new (uintptr_t(mStorage) + (alignment - (uintptr_t(mStorage) % alignment))) T[aCount];

and mPtr = new (mStorage) T[aCount]
Attachment #8428430 - Flags: review?(bas) → review-
Attached patch patchSplinter Review
Attachment #8428430 - Attachment is obsolete: true
Attachment #8437881 - Flags: review?(bas)
Comment on attachment 8437881 [details] [diff] [review]
patch

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

I feel we really want to make sure the compiler optimized out the loop for the destructors when they're no-ops. Otherwise we'll need some type specializations.

::: gfx/2d/Tools.h
@@ +93,5 @@
>  
>  template<typename T, int alignment = 16>
>  struct AlignedArray
>  {
> +  typedef T value_type;

nit: Do you really want this in here? You're not using it yourself.

@@ +145,2 @@
>      if (uintptr_t(mStorage) % alignment) {
>        // Our storage does not start at a <alignment>-byte boundary. Make sure mData does!

nit: While we're in this code let's fix the comment and make it say mPtr instead of mData.
Attachment #8437881 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/a6cf083123eb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment on attachment 8437881 [details] [diff] [review]
patch


[Approval Request Comment]
Uplifting this bug will make it possible to uplift bug 1063733.
Attachment #8437881 - Flags: approval-mozilla-b2g32?
Attachment #8437881 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Blocks: Woodduck
You need to log in before you can comment on or make changes to this bug.