I guess it really depends on whether TransformationMatrix needs the alignment. It's declared to want it on x86_64, but I haven't read the assembly code to see if it actually relies on/benefits from alignment.

I think suzyh@ was looking at the assembly in TransformationMatrix; maybe she knows.

I'm afraid not. I was only looking at testing TransformationMatrix to see whether its behaviour matched the comment, and don't actually know anything about the assembly code itself.

Saw the following in TransformationMatrix.h:

    void checkAlignment()
    {
#if defined(TRANSFORMATION_MATRIX_USE_X86_64_SSE2)
        // m_matrix can cause this class to require higher than usual alignment.
        // Make sure the allocator handles this.
        ASSERT((reinterpret_cast<uintptr_t>(this) & (WTF_ALIGN_OF(TransformationMatrix) - 1)) == 0);
#endif
    }

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/transforms/TransformationMatrix.h?q=TransformationMatrix&sq=package:chromium&l=425

Wondering why we didn't encounter this.

There might be also a risk of ContiguousContainer about that it doesn't align the starting address (Buffer::m_begin). I think it happens to work because new char[] ensures the maximum alignment of all possible types, and the alignment of TransfomationMatrix happens to be 16 which is maximum alignment (alignof(long double)), making Buffer::m_begin happens to be aligned for TransformationMatrix.

However, I'm not sure if all allocators follow the the maximum alignment rule (as the comment on TransformationMatrix says no allocator other than PartitionAllocator can be used), and how PartitionAllocator ensures this.

TransformationMatrix needs to be aligned. See  bug 534853  for background reading. I don't remember if we added the assert mentioned in comment 15 there -- if we haven't, we should.

Thanks thakis@ for the info.

Can you point me how PartitionAlloc ensures the alignment?

If other allocators don't ensure the alignment, then does this mean placement new won't work for TransformationMatrix on a buffer returned from malloc or new char[]?

All I remember is what is in the comments on that other bug.


"""The expressions new char[N] and new unsigned char[N] are guaranteed to return memory sufficiently aligned for any object. See §5.3.4/10 "[...] For arrays of char and unsigned char, the difference between the result of the new-expression and the address returned by the allocation function shall be an integral multiple of the strictest fundamental alignment requirement (3.11) of any object type whose size is no greater than the size of the array being created. [ Note: Because allocation functions are assumed to return pointers to storage that is appropriately aligned for objects of any type with fundamental alignment, this constraint on array allocation overhead permits the common idiom of allocating character arrays into which objects of other types will later be placed. —end note ]".""" -- http://stackoverflow.com/questions/10587879/does-new-char-actually-guarantee-aligned-memory-for-a-class-type

This is true for malloc too. It's not automatically true for stuff allocated on the static, but TransformationMatrix has attributes that make it true on the stack as well.

The only thing that can get this wrong are custom allocators that aren't careful.

Thanks for the quotation.

I read something similar when working on the bug. In a version of the patch (https://codereview.chromium.org/2119033003/diff/1/third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp) I had included a logic to align the starting address of the contiguous buffer before I noticed the rule and removed the logic. However, the logic seems still needed if the contiguous buffer is allocated by OilPan. (Is it still true that OilPan breaks the maximum alignment rule?)

BTW, I think the quotation means the alignment is ensured to be aligned at alignof(max_align_t) (i.e. alignof(long double)), but not ensured for any custom alignment. PartitionAllocator happens to work for TransformationMatrix because its custom alignment doesn't exceed alignof(max_align_t).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2bdd54edb30daa800142bf0e996eb75cb417e94

commit d2bdd54edb30daa800142bf0e996eb75cb417e94
Author: wangxianzhu <wangxianzhu@chromium.org>
Date: Wed Jul 06 23:37:22 2016

Correct alignment in ContiguousContainer::appendByMoving()

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG= 625381 
TBR=enne@chromium.org (for same change in cc as in blink)

Review-Url: https://codereview.chromium.org/2122573002
Cr-Commit-Position: refs/heads/master@{#403979}

[modify] https://crrev.com/d2bdd54edb30daa800142bf0e996eb75cb417e94/cc/base/contiguous_container.h
[modify] https://crrev.com/d2bdd54edb30daa800142bf0e996eb75cb417e94/cc/base/contiguous_container_unittest.cc
[modify] https://crrev.com/d2bdd54edb30daa800142bf0e996eb75cb417e94/third_party/WebKit/Source/platform/graphics/ContiguousContainer.h
[modify] https://crrev.com/d2bdd54edb30daa800142bf0e996eb75cb417e94/third_party/WebKit/Source/platform/graphics/ContiguousContainerTest.cpp

Yeah, Oilpan doesn't have an ability to adjust object alignment.

That's exactly why we have to put TransformationMatrix on PartitionAlloc (see this comment: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/transforms/TransformationMatrix.h?q=TransformationMatrix&sq=package:chromium&dr=CSs&l=52).