chore(allocator): make and makeArray should allocate aligned memory

[ljmf00]

Signed-off-by: Luís Ferreira [email protected]

[dlang-bot]

Thanks for your pull request, @ljmf00!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8577"

[ljmf00]

Humm, why does RCIAllocator skip zeroed allocation method, but not aligned allocation, that is also optional? I think this is still badly designed, unfortunately. A generic allocator may only assume methods that are required to be implemented on every allocator, otherwise we rely on special return values that are both used for errors on allocation and not implemented.

[ljmf00]

Do we really need this? If make/makeArray is instantiated with AlignedMallocator then all of this becomes moot.

Yes, to improve correctness. I would expect correct alignment when providing AlignedMallocator to makeArray.

[atilaneves]

Do we really need this? If make/makeArray is instantiated with AlignedMallocator then all of this becomes moot.

Yes, to improve correctness. I would expect correct alignment when providing AlignedMallocator to makeArray.

Does that not happen now?

[ljmf00]

Do we really need this? If make/makeArray is instantiated with AlignedMallocator then all of this becomes moot.

Yes, to improve correctness. I would expect correct alignment when providing AlignedMallocator to makeArray.

Does that not happen now?

No. That's what this PR is trying to solve, although, the design of the allocators doesn't allow doing it efficiently. It requires checking for null on Allocators wrapped in an interface.

See:

struct S
{
align(512):
    int a;
    float b;
    int c;
}

S* s1 = new S();
assert(cast(size_t)s1 % 512 == 0, "Misaligned"); // fine
S* s2 = AlignedMallocator.instance.make!S();
assert(cast(size_t)s2 % 512 == 0, "Misaligned"); // triggers

[RazvanN7]

@ljmf00 This seems to be a problem of AlignedMallocator. It seems that the alignment is hardcoded to be the platform alignment. I think that AlignedMallocator should just have an instance dependent alignment. Of course, that does not bode well with the current pattern of using a static shared instance of it. Is there anything preventing us from allowing AlignedMallocator from being used from different, independent instances that have their own alignment? I think that this is a more general solution compared to the one in this PR.

[ljmf00]

@ljmf00 This seems to be a problem of AlignedMallocator. It seems that the alignment is hardcoded to be the platform alignment. I think that AlignedMallocator should just have an instance dependent alignment. Of course, that does not bode well with the current pattern of using a static shared instance of it. Is there anything preventing us from allowing AlignedMallocator from being used from different, independent instances that have their own alignment? I think that this is a more general solution compared to the one in this PR.

Why are you required to create new allocators for each alignment sizes? I'm not getting your suggestion. Maybe you can ellaborate, but from my understanding that is not the intended way.

There's a defined optional function that an allocator can define called alignedAllocate that provides that capability.

The alignment property you mentioned is to describe the alignment of memory allocated with allocate function. It is hardcoded because it uses an underlying malloc for normal "unaligned" allocations (provided by allocate method), which defaults to platform alignment.

The fact that the generic allocator wrapper, provides a alignedAllocate, is, in my opinion wrong design. Now, to support this correctly and/or efficient, we need to either check for null or add special cases.

IMO, a generic allocator should only provide strictly mandatory functions to be implemented by any allocator. With the current behavior, no one is going to use alignedAllocate in performant code, because apart from calling a virtual function, it might yield null at runtime and you have to do additional branching to do the fallback correctly. As much as possible, the existence of this method should be checked or encouraged to be checked at compile-time.