Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented Memory Provider for Heterogeneous memory. #674

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vinser52
Copy link

The idea is to add abstraction layer for heterogeneous memory.
The interface is built on top of C++ 17 std::pmr::memory_resource
interface.

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@misiugodfrey misiugodfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@paul-aiyedun paul-aiyedun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1,6 @@
set(heteromem_source_files
MemResourceProvider.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the new files to a directory under DataMgr/Allocators.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

CMakeLists.txt Outdated
@@ -332,6 +332,10 @@ else()
endif()
endif()

set(MEMKIND_REQUIRED_VERSION 1.11.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,156 @@
// SPDX-License-Identifier: BSD-2-Clause
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the OmniSci license in all new files (see an example here).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

using pmem_memory_resource_type = libmemkind::pmem::memory_resource;
using static_kind_memory_resource_type = libmemkind::static_kind::memory_resource;

MemoryResourceProvider::MemoryResourceProvider()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to install Memkind dependencies.
Also please install libkmod-dev

@vinser52 vinser52 force-pushed the memory_provider branch 2 times, most recently from 694776c to bc052f1 Compare September 3, 2021 10:44
The idea is to add abstraction layer for heterogeneous memory.
The interface is built on top of C++ 17 std::pmr::memory_resource
interface.
@alexbaden
Copy link
Contributor

Note that we plan to move to gcc-9 imminently. Dependencies scripts have already been updated.

@alexbaden
Copy link
Contributor

@misiugodfrey @paul-aiyedun @vinser52 what is the status of this PR (cc @Garra1980)?

@vinser52
Copy link
Author

@alexbaden Sorry for the delayed response. I just noticed your comments.
Regarding gcc-9, sounds good.
As for the status, @misiugodfrey and @paul-aiyedun are working on integrating these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants