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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

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. ;)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

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.

Comment 2

[Bas Schouten (:bas.schouten)]

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]

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 4

[Bas Schouten (:bas.schouten)]

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.

Comment 5

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6cf083123eb

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a6cf083123eb

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 8437881 [details] [diff] [review]
patch


[Approval Request Comment]
Uplifting this bug will make it possible to uplift bug 1063733.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/40a73e349ab5

Comment 9

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/40a73e349ab5