Bug 19946

Created attachment 55726 [details] Apply appropriate defines for ARMv5TE architectures so we can force 8 byte alignment in object allocators.

Darin, can you have a look?

Comment on attachment 55726 [details] Apply appropriate defines for ARMv5TE architectures so we can force 8 byte alignment in object allocators. > +#ifdef WTF_ARMV5_NEEDS_ALIGN_8 > +#define ARENA_ALIGN_MASK 7 > +#else > #define ARENA_ALIGN_MASK 3 > +#endif I'd like to see us do in a way that's a bit more automatic. One way is: #define ARENA_ALIGN_MASK (sizeof(AllocAlignmentInteger) - 1) All that ARM-specific stuff seems undesirable. In fact, other 64-bit platforms would probably benefit from 64-bit-aligned objects in the arenas. > +#define ARENA_ALIGNED_HEADER_SIZE ((sizeof(RenderArenaDebugHeader) + ARENA_ALIGN_MASK) & ~ARENA_ALIGN_MASK) We should use the ARENA_ALIGN macro here. We can change it so it doesn't take a "pool" argument and then use it like this: ARENA_ALIGN(sizeof(RenderArenaDebugHeader)) Does this need to be a macro? This is C++ so I'd think we could just use "static const" instead. > - RenderArenaDebugHeader* header = static_cast<RenderArenaDebugHeader*>(ptr) - 1; > + void* block = static_cast<unsigned char*>(ptr) - ARENA_ALIGNED_HEADER_SIZE; Using "unsigned char*" here instead of "char*" is a bit strange. Change seems OK, but I think it would be better to do this without so much ARM-specific. review- for now Please let me know if I misunderstood the basis of the need for this on ARM. Is it needed on 32-bit ARM?

Yes we do require this on 32 bit ARMv5TE. ARMv5TE has double word instructions LDRD and STRD that load 64bits in a single instruction; but the requirement is that the dest/src need to be 8 byte aligned. The problem is that we are running allocators from pools that are 4 byte aligned. This is all well and good for on most platforms that can do double word loads from 4 byte addresses; but that CPU can't. I agree that this would be good on all platforms to move to 8 byte alignment on objects to move them more inline with what a malloc would provide; but didn't want to influence all other platforms for this one CPU case. I'll prepare a patch as per your comments.

(In reply to comment #4) > Yes we do require this on 32 bit ARMv5TE. I think you may be right in not wanting to affect the other platforms, but Platform.h should be indicating that the alignment is needed in a way that is not ARM-specific, so not "WTF_ARMV5_NEEDS_ALIGN_8".

Created attachment 56069 [details] Generalize last patch removing ARM specific stuff.

Attachment 56069 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/Arena.h:45: Alphabetical sorting problem. [build/include_order] [4] WebCore/platform/Arena.h:83: _nb is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/Arena.h:94: _incr is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 56070 [details] Add alignment based on AllocAlignmentInteger for Arenas.

Attachment 56070 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/Arena.h:83: _nb is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/Arena.h:94: _incr is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 56070 [details] Add alignment based on AllocAlignmentInteger for Arenas. > +#include <wtf/FastMalloc.h> > +#include <wtf/Platform.h> Since FastMalloc.h already includes Platform.h, there is no need to include Platform.h here. > +#ifdef WTF_USE_ARENA_ALLOC_ALIGNMENT_INTEGER > +#define ARENA_ALIGN_MASK (sizeof(WTF::AllocAlignmentInteger) - 1) > +#else > #define ARENA_ALIGN_MASK 3 > +#endif I think it would be good eventually to get rid of WTF_USE_ARENA_ALLOC_ALIGNMENT_INTEGER and always do this. A FIXME mentioning that would be a plus. r=me

Created attachment 56079 [details] Add alignment based on AllocAlignmentInteger for Arenas

Attachment 56079 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/Arena.h:84: _nb is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/Arena.h:95: _incr is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 56079 [details] Add alignment based on AllocAlignmentInteger for Arenas Clearing flags on attachment: 56079 Committed r59525: <http://trac.webkit.org/changeset/59525>

All reviewed patches have been landed. Closing bug.