Bug 65651 – Make sure that AudioArray is 16-byte aligned

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Make sure that AudioArray is 16-byte aligned

[Chris Rogers]

Created attachment 102846 [details]
Patch

[Chris Rogers]

this fixes:
https://bugs.webkit.org/show_bug.cgi?id=63998

Audio arrays should be 16-byte aligned for maximum performance.  In FFTFrameFFMPEG's case, the av_rdft_calc() function actually can crash if not given an aligned address, although this is not documented!

[Kenneth Russell]

Comment on attachment 102846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102846&action=review

Generally looks good but I strongly feel you should be using FastMalloc.h.

> Source/WebCore/platform/audio/AudioArray.h:48
> +        free(m_allocation);

You should use fastMalloc and fastFree (in <wtf/FastMalloc.h>), as Vector.h does, to have the possibility of using WebKit's malloc validation, and to crash upon allocation failure on all platforms.

> Source/WebCore/platform/audio/AudioArray.h:59
> +            free(m_allocation);

Here too.

> Source/WebCore/platform/audio/AudioArray.h:68
> +            T* allocation = static_cast<T*>(malloc(sizeof(T) * n + extraAllocationBytes));

Here too.

> Source/WebCore/platform/audio/AudioArray.h:79
> +                free(allocation);

Here too.

[Chris Rogers]

Created attachment 102954 [details]
Patch

[Chris Rogers]

Comment on attachment 102846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102846&action=review

>> Source/WebCore/platform/audio/AudioArray.h:48
>> +        free(m_allocation);
> 
> You should use fastMalloc and fastFree (in <wtf/FastMalloc.h>), as Vector.h does, to have the possibility of using WebKit's malloc validation, and to crash upon allocation failure on all platforms.

FIXED

>> Source/WebCore/platform/audio/AudioArray.h:59
>> +            free(m_allocation);
> 
> Here too.

FIXED

>> Source/WebCore/platform/audio/AudioArray.h:68
>> +            T* allocation = static_cast<T*>(malloc(sizeof(T) * n + extraAllocationBytes));
> 
> Here too.

FIXED

>> Source/WebCore/platform/audio/AudioArray.h:79
>> +                free(allocation);
> 
> Here too.

FIXED

[Kenneth Russell]

Comment on attachment 102954 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 102954 [details]
Patch

Clearing flags on attachment: 102954

Committed r92408: <http://trac.webkit.org/changeset/92408>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Berend-Jan Wever]

Sorry I'm late to the party. I have some questions and remarks about the patch. I assume you added the loop in an attempt to prevent over-allocating to align the data in case it aligns correctly by chance.

1) Since allocations are already 4 bytes aligned, you will need to add 4, 8 or 12 bytes to correct the alignment if it's not aligned correctly. Why are you adding "alignment" (16 bytes) to the allocation, rather than "alignment - 4"? Your code over-allocates 12, 8 or 4 bytes for an average of 6 bytes. If you use "alignment - 4", this will drop to 3 bytes.

2) Why use a loop when the code is only going to be executed twice? It seems the code would be much cleaner if you unrolled the loop. Also, why use a local variable to terminate it, rather than break?

m_allocation = static_cast<T*>(fastMalloc(sizeof(T) * n)); 
m_alignedData = alignedAddress(m_allocation, alignment); 

if (m_alignedData != m_allocation) { 
  fastFree(m_allocation); 
  m_allocation = static_cast<T*>(fastMalloc(sizeof(T) * n + alignment - 4));
  m_alignedData = alignedAddress(m_allocation, alignment); 
}
m_size = n; 
zero(); 

3) If you immediately over-allocate "alignment - 4", there is no need for a "loop" or "if" at all, simplifying the code even further. In this case you will over allocate 12, 8, 4 or 0 bytes for an average of 6, which is equivalent to the current code.

m_allocation = static_cast<T*>(fastMalloc(sizeof(T) * n + alignment - 4));
m_alignedData = alignedAddress(m_allocation, alignment); 
m_size = n; 
zero();