Add Allocator support to executors

[upsj]

This adds support for custom allocators to our executors (except for DPC++). We design follows the malloc/free function pair, with base types for all executor memory spaces to avoid users accidentally using incompatible Executor/Allocator pairs. I also used this opportunity to clean up the executor.* files and separate forward-declarations, streams, allocators and executors into different files, and deprecate the device_reset flag, which is no longer necessary for tests.

Why model malloc/free and not more allocator-friendly interfaces?

Without an interface-break, we cannot change the raw_alloc/raw_free interface, so we need to stick with the minimum that can be callsed from raw_alloc(size) and raw_free(pointer). Additionally, none of the allocator wrappers I implemented can profit from any additional size information or even an opaque allocator info struct.

[MarcelKoch]

Since there were also other design options for the allocator interface, would you mind giving a very brief explanation, why you chose this approach over the others?

[upsj]

@MarcelKoch Good point, I added an explanation - basically interface stability. Also none of the allocator wrappers I implemented can profit from any additional information.

[yhmtsai]

should it use deallocate or just free?

[upsj]

this is using the nomenclature from https://en.cppreference.com/w/cpp/memory/allocator

[yhmtsai]

should we keep the behavior although it's not part of the interface?

[upsj]

if we no longer call DeviceReset, I think we should only return false to make it clearer to users. The deprecation warnings already do most of the job though.

[yhmtsai]

cudaAllocator will not take care of the device_id. (not unified one)
should it take care of device_id?
i.e. is the CudaAllocator the global allocator like cudaMalloc/cudaFree or device specified allocator?

[upsj]

I don't see extremely strong arguments either way, but this was the approach that required less responsibility on the allocator and less code in general.

[MarcelKoch]

LGTM, but I think the PR should not be merged until someone who as a use-case from an application looked at it. I have one larger point regarding the creation of the allocators, otherwise only smaller nits.

[upsj]

I agree with your suggestion to sync with application users, I'll see if I can find somebody with suitable experience. ATM, I have two possible interface variants in mind to deal with the device_id/stream_handle mismatch potential:

1. stateful allocators with allocate(size_type)/deallocate(void*) that store stream handle and maybe device ID to allow unified CPU/GPU allocation directly. Can lead to mismatches, which we may need to check for at executor construction time?
2. (by default) stateless allocators with allocate(stream_handle, size_type)/deallocate(stream_handle, void*) for GPUs and without stream_handle for CPU. This makes mismatches impossible, but makes it harder to use the same allocator for CPU and GPU.

ATM, I am leaning towards 1. i.e. adding get_device_id and is_stream_ordered/get_stream functions so that executors can check at construction time whether the allocator was set up correctly. What do you think @MarcelKoch?

[MarcelKoch]

I was too subtle with my remark, sorry about that. I just meant that @pratikvn should take a look at it, because he mentioned that he has a use case for custom allocators.

[MarcelKoch]

I think 1. is a suitable approach. But then we should add the checks to the executor constructor.
Another approach could be to have intermediate classes which have a function like std::unique_ptr<Allocator> create_allocator(const Executor*), but that might complicate the class hierarchy a bit and thus make it harder for users to provide custom allocators.

[MarcelKoch]

LGTM, only one minor nit.

Regarding the user application, I think @pratikvn mentioned Amrex as a use case: https://amrex-codes.github.io/amrex/docs_html/GPU.html#memory-allocation. From the looks of it, it would match pretty well.

[yhmtsai]

how to call DeviceReset safely actually?
I assume we need to call it after all device-related objects are deleted. Thus, need another scope as following, is it right?

int main() {
{// using ginkgo including executor construction}
DeviceReset();
}

I also add @pratikvn into the reviewer list to ensure it fits the application needs.

[pratikvn]

I had a look at what applications may need, with SUNDIALS as an example. It looks like this interface should be sufficient.

These applications would need to create their own allocators deriving from XAllocatorBase for the different backends (Cuda, Hip etc) and pass that into executor object where they create the ginkgo executors. These allocators of course override the allocate and deallocate functions.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
83 Code Smells

21.2% Coverage
3.2% Duplication