Bug 16925 – How to fix VectorBuffer's alignment FIXME

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Marc-Antoine Ruel]

To remove the FIXME for VectorBuffer<>::m_inlineBuffer at line 381, you should define the alignment, which is a compiler-specific attribute. That would help for Vector<double> and would be needed to use with SSE primitives.

For GCC, I think that would be:
  char __attribute__((aligned(__alignof__(T)))) m_inlineBuffer[m_inlineBufferSize];

For MSVC, I think that would be:
  char __declspec(align(__alignof(T))) m_inlineBuffer[m_inlineBufferSize];

For sure, a macro would need to be defined to not directly clutter the code. Since I don't know where to put the necessary macros, I'll let more experimented folks figure that out.

[Alexey Proskuryakov]

See also: bug 19775.

[Maciej Stachowiak]

I think more portable than this approach would be to union the char array with a one-element array of type T.

[Maciej Stachowiak]

Oliver reminds me that this doesn't work if T has a constructor. alignof it is then.

[Paul Pedriana]

Created attachment 23467 [details]
Patch which resolves this problem.

This was recently discussed on the webkit-dev mailing list at a thread starting here: https://lists.webkit.org/pipermail/webkit-dev/2008-September/004777.html

[Darin Adler]

Comment on attachment 23467 [details]
Patch which resolves this problem.

Looks good! Thanks for tackling this.

+#define WTF_ALIGN_OF(type)   __alignof__(type)
+#define WTF_ALIGNED(variable_type, variable, n) variable_type variable __attribute__((aligned(n)))

These macros are fine, but Platform.h is the wrong header for them. It's a configuration header only and not the place to put macros.

We could add a new header like <wtf/AlwaysInline.h> or find some other header to put them in. For now, it would be fine for them to be in Vector.h with the intention of moving them elsewhere later.

+        typedef char aligned_buffer_char; 

This is the wrong naming style for WebKit code. The type should be named something like AlignedBufferChar.

+    template <size_t size, size_t alignment>
+    struct aligned_buffer { aligned_buffer_char buffer[size]; };

Same comment here.

We should fix the "wrong header" and naming issues, and then someone should test to be sure this doesn't slow down performance, and then we can land this.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Created an attachment (id=23467) [edit]
> Patch which resolves this problem.

A couple additional comments:

1. Please use the COMPILER() macros when checking for compilers (except for version checks).  A new macro will need to be added for Metrowerks.

2. I think alignment macros need to be added for the mingw compiler before the patch lands.  I think one of the existing ports uses it (Qt for Windows?), and it would break their build if it was not supported.

[Paul Pedriana]

Created attachment 23487 [details]
Revised patch

This patch:
   - moves the align macros back to vector.h
   - renames aligned_buffer_char to AlignedBufferChar
   - renamed aligned_buffer to aligned_buffer
   - used #if COMPILER(GCC) instead of #if defined(__GNUC___)
   - adds COMPILER(MINGW)
   - removes the reference to __MWERKS__ for now.

[Darin Adler]

Comment on attachment 23487 [details]
Revised patch

r=me

[Darin Adler]

http://trac.webkit.org/changeset/36740