Implemented Memory Provider for Heterogeneous memory. #674
Conversation
|
|
vinser52
force-pushed
the
memory_provider
branch
from
0902a18
to
0390430
Compare
There was a problem hiding this comment.
Overall looks good to me.
The only potential issue I see is with the step up to gcc-9 requirements, which as I understand it is required for the <memory_resource> header.
I'm not sure what would be required on our end right now to make that dependency upgrade, but to keep the scope down for the current work, is it reasonable/possible to avoid the dependency on memory_resource and just implement the Arena interface (DataMgr/Allocators/ArenaAllocator.h) for the the new memory types?
There was a problem hiding this comment.
The overall approach looks good to me. However, we should be able to simplify the integration and reduce the scope of work by making the allocator a specialization of the Arena class. I also had issues when attempting to install memkind using the script in the PR.
HeteroMem/CMakeLists.txt
Outdated
| @@ -0,0 +1,6 @@ | |||
| set(heteromem_source_files | |||
| MemResourceProvider.cpp) | |||
There was a problem hiding this comment.
Please move the new files to a directory under DataMgr/Allocators.
CMakeLists.txt
Outdated
| @@ -332,6 +332,10 @@ else() | |||
| endif() | |||
| endif() | |||
|
|
|||
| set(MEMKIND_REQUIRED_VERSION 1.11.0) | |||
There was a problem hiding this comment.
I think we should put all the memkind changes behind a new (ENABLE_MEMKIND) compile flag, so we can iteratively integrate this dependency without much disruption.
HeteroMem/MemResourceProvider.cpp
Outdated
| @@ -0,0 +1,156 @@ | |||
| // SPDX-License-Identifier: BSD-2-Clause | |||
There was a problem hiding this comment.
Please use the OmniSci license in all new files (see an example here).
There was a problem hiding this comment.
As we discussed earlier, we are going to move this functionality into a separate library. Should we still use OmniSci license? Especially for memory_resources.h?
HeteroMem/MemResourceProvider.cpp
Outdated
| using pmem_memory_resource_type = libmemkind::pmem::memory_resource; | ||
| using static_kind_memory_resource_type = libmemkind::static_kind::memory_resource; | ||
|
|
||
| MemoryResourceProvider::MemoryResourceProvider() |
There was a problem hiding this comment.
For the initial integration, we should be able to start with something simpler, like a provider class that returns an instance of an Arena derived class. This would align with the interface that the buffer manager currently expects and avoid the additional scope of work introduced by the compiler version change.
There was a problem hiding this comment.
We are going to build a generic interface and decided to use C++ compliant interface. If you want you can build an Arena-like interface for OmniSci purposes.
| @@ -0,0 +1,44 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
I get an error when attempting to run this script on both Ubuntu and CentOS (even after installing the dependencies mentioned here). Are there other steps that need to be taken before running the script?
There was a problem hiding this comment.
you need to install Memkind dependencies.
Also please install libkmod-dev
vinser52
force-pushed
the
memory_provider
branch
2 times, most recently
from
694776c
to
bc052f1
Compare
The idea is to add abstraction layer for heterogeneous memory. The interface is built on top of C++ 17 std::pmr::memory_resource interface.
vinser52
force-pushed
the
memory_provider
branch
from
bc052f1
to
ed85c72
Compare
|
Note that we plan to move to gcc-9 imminently. Dependencies scripts have already been updated. |
|
@misiugodfrey @paul-aiyedun @vinser52 what is the status of this PR (cc @Garra1980)? |
|
@alexbaden Sorry for the delayed response. I just noticed your comments. |
The idea is to add abstraction layer for heterogeneous memory.
The interface is built on top of C++ 17 std::pmr::memory_resource
interface.