Summary: | How to fix VectorBuffer's alignment FIXME | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marc-Antoine Ruel <maruel> | ||||||
Component: | Web Template Framework | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, ddkilzer, ggaren, mjs | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Marc-Antoine Ruel
2008-01-18 13:18:35 PST
I think more portable than this approach would be to union the char array with a one-element array of type T. Oliver reminds me that this doesn't work if T has a constructor. alignof it is then. 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 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.
(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. 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.
Comment on attachment 23487 [details]
Revised patch
r=me
|