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.

Sign up for GitHub

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

huge paging #60

Merged
merged 2 commits into from
Dec 8, 2024
Merged

huge paging #60

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
48948bd
huge paging
PhilipDeegan Oct 8, 2024
e8bf861
++
PhilipDeegan Dec 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 82 additions & 4 deletions inc/mkn/kul/alloc.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <cstdlib>
#include <type_traits>

#include <limits>
#include <stdlib.h> // posix_memalign
#include <sys/mman.h> // madvise

namespace mkn::kul {

template <typename T, std::int32_t alignment = 32>
Expand Down Expand Up @@ -98,10 +102,7 @@ class Allocator {
using other = Allocator<U>;
};

T* allocate(std::size_t const n) const {
if (n == 0) return nullptr;
return static_cast<T*>(::operator new(n * sizeof(T)));
}
T* allocate(std::size_t const n) const { return static_cast<T*>(::operator new(n * sizeof(T))); }

void deallocate(T* const p) noexcept {
if (p) ::operator delete(p);
Expand All @@ -117,16 +118,93 @@ class Allocator {

template <typename T>
class NonConstructingAllocator : public Allocator<T> {
using This = NonConstructingAllocator<T>;

public:
template <typename U>
struct rebind {
using other = NonConstructingAllocator<U>;
};

T* allocate(std::size_t const n) const {
// if (n == 0) return nullptr;
return static_cast<T*>(malloc(n * sizeof(T)));
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure proper handling of zero-size allocations and allocation failures

In the NonConstructingAllocator::allocate method:

  • The check for zero-size allocation (if (n == 0) return nullptr;) is commented out. Allocating zero elements can lead to undefined behavior. It's advisable to reintroduce this check to handle zero-size allocations safely.
  • Using malloc instead of ::operator new bypasses exception handling on allocation failure. malloc returns nullptr on failure, which might not throw a std::bad_alloc exception as expected in standard allocator behavior.

Apply this diff to address these concerns:

+      if (n == 0) return nullptr;
       return static_cast<T*>(malloc(n * sizeof(T)));
+      if (!p) throw std::bad_alloc();

Consider using ::operator new to maintain consistency with standard allocation practices, unless there is a specific reason to use malloc.

Committable suggestion skipped: line range outside the PR's diff.


template <typename U, typename... Args>
void construct(U* /*ptr*/, Args&&... /*args*/) {} // nothing
template <typename U>
void construct(U* /*ptr*/) noexcept(std::is_nothrow_default_constructible<U>::value) {}

bool operator!=(This const& that) const { return !(*this == that); }
bool operator==(This const& /*that*/) const {
return true; // stateless
}
};

template <typename T, std::size_t huge_page_size = 4096>
class HugePageAllocator : public Allocator<T> {
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Set huge_page_size to a typical huge page size

Currently, huge_page_size is set to 4096 bytes, which is the standard page size on most systems. Huge pages are typically 2MB or larger. Setting huge_page_size to a standard huge page size will ensure that the allocator benefits from huge page optimizations.

Apply this diff to adjust the default huge page size:

-template <typename T, std::size_t huge_page_size = 4096>
+template <typename T, std::size_t huge_page_size = 2 * 1024 * 1024>
 class HugePageAllocator : public Allocator<T> {

And similarly for NonConstructingHugePageAllocator:

-template <typename T, std::size_t huge_page_size = 4096>
+template <typename T, std::size_t huge_page_size = 2 * 1024 * 1024>
 class NonConstructingHugePageAllocator : public NonConstructingAllocator<T> {

Alternatively, consider making huge_page_size a runtime parameter or detecting the system's huge page size dynamically.

Also applies to: 174-175

using This = HugePageAllocator<T, huge_page_size>;

public:
template <typename U>
struct rebind {
using other = HugePageAllocator<U>;
};

HugePageAllocator() = default;
template <class U>
constexpr HugePageAllocator(HugePageAllocator<U> const&) noexcept {}

T* allocate(std::size_t n) {
if (n > std::numeric_limits<std::size_t>::max() / sizeof(T)) throw std::bad_alloc();
void* p = nullptr;
posix_memalign(&p, huge_page_size, n * sizeof(T));
madvise(p, n * sizeof(T), MADV_HUGEPAGE);
if (p == nullptr) throw std::bad_alloc();
return static_cast<T*>(p);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct error handling for posix_memalign and avoid potential undefined behavior

In the HugePageAllocator::allocate method:

  • posix_memalign returns zero on success and a non-zero error code on failure. The value of p is undefined if allocation fails. Checking p for nullptr is not reliable.
  • madvise should only be called if the allocation was successful to avoid undefined behavior.

Apply this diff to fix the error handling:

       void* p = nullptr;
-      posix_memalign(&p, huge_page_size, n * sizeof(T));
-      madvise(p, n * sizeof(T), MADV_HUGEPAGE);
-      if (p == nullptr) throw std::bad_alloc();
+      int rc = posix_memalign(&p, huge_page_size, n * sizeof(T));
+      if (rc != 0) throw std::bad_alloc();
+      madvise(p, n * sizeof(T), MADV_HUGEPAGE);
       return static_cast<T*>(p);

This ensures that madvise is only called when memory allocation succeeds and that the proper exception is thrown on failure.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
T* allocate(std::size_t n) {
if (n > std::numeric_limits<std::size_t>::max() / sizeof(T)) throw std::bad_alloc();
void* p = nullptr;
posix_memalign(&p, huge_page_size, n * sizeof(T));
madvise(p, n * sizeof(T), MADV_HUGEPAGE);
if (p == nullptr) throw std::bad_alloc();
return static_cast<T*>(p);
}
T* allocate(std::size_t n) {
if (n > std::numeric_limits<std::size_t>::max() / sizeof(T)) throw std::bad_alloc();
void* p = nullptr;
int rc = posix_memalign(&p, huge_page_size, n * sizeof(T));
if (rc != 0) throw std::bad_alloc();
madvise(p, n * sizeof(T), MADV_HUGEPAGE);
return static_cast<T*>(p);
}

void deallocate(T* p, std::size_t n) { std::free(p); }
bool operator!=(This const& that) const { return !(*this == that); }
bool operator==(This const& /*that*/) const {
return true; // stateless
}
};

template <typename T, std::size_t huge_page_size = 4096>
class NonConstructingHugePageAllocator : public NonConstructingAllocator<T> {
using This = NonConstructingHugePageAllocator<T, huge_page_size>;

public:
template <typename U>
struct rebind {
using other = NonConstructingHugePageAllocator<U>;
};

NonConstructingHugePageAllocator() = default;
template <class U>
constexpr NonConstructingHugePageAllocator(NonConstructingHugePageAllocator<U> const&) noexcept {}

T* allocate(std::size_t n) {
if (n > std::numeric_limits<std::size_t>::max() / sizeof(T)) throw std::bad_alloc();
void* p = nullptr;
posix_memalign(&p, huge_page_size, n * sizeof(T));
madvise(p, n * sizeof(T), MADV_HUGEPAGE);
if (p == nullptr) throw std::bad_alloc();
return static_cast<T*>(p);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure proper error handling in NonConstructingHugePageAllocator::allocate

The same concerns apply here as in the HugePageAllocator:

  • Check the return value of posix_memalign instead of the p pointer.
  • Call madvise only if the allocation is successful to prevent undefined behavior.

Apply this diff:

       void* p = nullptr;
-      posix_memalign(&p, huge_page_size, n * sizeof(T));
-      madvise(p, n * sizeof(T), MADV_HUGEPAGE);
-      if (p == nullptr) throw std::bad_alloc();
+      int rc = posix_memalign(&p, huge_page_size, n * sizeof(T));
+      if (rc != 0) throw std::bad_alloc();
+      madvise(p, n * sizeof(T), MADV_HUGEPAGE);
       return static_cast<T*>(p);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
T* allocate(std::size_t n) {
if (n > std::numeric_limits<std::size_t>::max() / sizeof(T)) throw std::bad_alloc();
void* p = nullptr;
posix_memalign(&p, huge_page_size, n * sizeof(T));
madvise(p, n * sizeof(T), MADV_HUGEPAGE);
if (p == nullptr) throw std::bad_alloc();
return static_cast<T*>(p);
}
T* allocate(std::size_t n) {
if (n > std::numeric_limits<std::size_t>::max() / sizeof(T)) throw std::bad_alloc();
void* p = nullptr;
int rc = posix_memalign(&p, huge_page_size, n * sizeof(T));
if (rc != 0) throw std::bad_alloc();
madvise(p, n * sizeof(T), MADV_HUGEPAGE);
return static_cast<T*>(p);
}


void deallocate(T* p, std::size_t n) { std::free(p); }

template <typename U, typename... Args>
void construct(U* /*ptr*/, Args&&... /*args*/) {} // nothing
template <typename U>
void construct(U* /*ptr*/) noexcept(std::is_nothrow_default_constructible<U>::value) {}

bool operator!=(This const& that) const { return !(*this == that); }
bool operator==(This const& /*that*/) const {
return true; // stateless
}
};

} // namespace mkn::kul
Expand Down
30 changes: 21 additions & 9 deletions inc/mkn/kul/vector.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,26 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#ifndef _MKN_KUL_VECTOR_HPP_
#define _MKN_KUL_VECTOR_HPP_

#include "alloc.hpp"
#include "mkn/kul/alloc.hpp"

#include <vector>
#include <type_traits>

namespace mkn::kul {

template <typename T, typename A = void> // A ignored but there for std::vector interop
template <typename T> // A ignored but there for std::vector interop
using Vector = std::vector<T, Allocator<T>>;

template <typename T> // A ignored but there for std::vector interop
using NonConstructingVector = std::vector<T, NonConstructingAllocator<T>>;

template <typename T, std::size_t S = 4096>
using NonConstructingHugePageVector = std::vector<T, NonConstructingHugePageAllocator<T, S>>;

template <typename T, std::size_t S = 4096>
using HugePageVector = std::vector<T, HugePageAllocator<T, S>>;

template <typename T>
std::vector<T, Allocator<T>>& as_super(std::vector<T, NonConstructingAllocator<T>>& v) {
return *reinterpret_cast<std::vector<T, Allocator<T>>*>(&v);
Expand All @@ -51,26 +62,27 @@ std::vector<T, Allocator<T>> const& as_super(std::vector<T, NonConstructingAlloc

} // namespace mkn::kul

template <typename T, typename A0, typename A1 = void>
bool operator==(std::vector<T, A0> const& v0, mkn::kul::NonConstructingVector<T, A1> const& v1) {
template <typename T, typename A0, typename A1,
std::enable_if_t<std::is_base_of_v<mkn::kul::Allocator<T>, A1>, bool> = 0>
bool operator==(std::vector<T, A0> const& v0, std::vector<T, A1> const& v1) {
if (v0.size() != v1.size()) return false;
for (std::size_t i = 0; i < v0.size(); i++)
if (v0[i] != v1[i]) return false;
return true;
}

template <typename T, typename A0, typename A1 = void>
bool operator==(mkn::kul::NonConstructingVector<T, A1> const& v0, std::vector<T, A0> const& v1) {
template <typename T, typename A0>
bool operator==(mkn::kul::Vector<T> const& v0, std::vector<T, A0> const& v1) {
return v1 == v0;
}

template <typename T, typename A0, typename A1 = void>
bool operator!=(std::vector<T, A0> const& v0, mkn::kul::NonConstructingVector<T, A1> const& v1) {
template <typename T, typename A0>
bool operator!=(std::vector<T, A0> const& v0, mkn::kul::Vector<T> const& v1) {
return !(v0 == v1);
}

template <typename T, typename A0, typename A1 = void>
bool operator!=(mkn::kul::NonConstructingVector<T, A1> const& v0, std::vector<T, A0> const& v1) {
template <typename T, typename A0>
bool operator!=(mkn::kul::Vector<T> const& v0, std::vector<T, A0> const& v1) {
return !(v0 == v1);
}

Expand Down
13 changes: 9 additions & 4 deletions mkn.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#! clean build -dtOp test
#! clean build -dtKOp test

# mkn.kul - Kommon Usage Library
# Cross platform wrapper for systems operations / IO / threads / processes
Expand Down Expand Up @@ -28,16 +28,21 @@ profile:
nix: ./src/os/nixish
arg: -D_MKN_KUL_COMPILED_LIB_

- name: test

- name: _test
parent: lib
inc: .
main: test/test.cpp
src: test/test
mode: none
dep: google.test
if_arg:
win_shared: -DGTEST_LINKED_AS_SHARED_LIBRARY=1

- name: test
parent: _test
main: test/test.cpp
src: test/test
mode: none

- name: bench
parent: lib
main: test/bench.cpp
Expand Down
67 changes: 52 additions & 15 deletions test/test/no_construct_alloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <array>
#include <cstddef>
#include <cstring>
#include <algorithm>
#include <stdexcept>

Expand Down Expand Up @@ -61,6 +62,27 @@ auto copy_manual(V const& v) {
return out;
}

template <typename V>
auto copy_mem(V const& v) {
KUL_DBG_FUNC_ENTER;
V out;
out.reserve(v.capacity());
out.resize(v.size());
memcpy(out.data(), v.data(), v.size() * sizeof(typename V::value_type));
return out;
}
Comment on lines +65 to +73
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add type trait check for safe memcpy usage.

The copy_mem function uses memcpy which is only safe for trivially copyable types. Add a static assertion to prevent misuse with non-trivially-copyable types.

 template <typename V>
 auto copy_mem(V const& v) {
   KUL_DBG_FUNC_ENTER;
+  static_assert(std::is_trivially_copyable_v<typename V::value_type>,
+                "copy_mem can only be used with trivially copyable types");
   V out;
   out.reserve(v.capacity());
   out.resize(v.size());
   memcpy(out.data(), v.data(), v.size() * sizeof(typename V::value_type));
   return out;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
template <typename V>
auto copy_mem(V const& v) {
KUL_DBG_FUNC_ENTER;
V out;
out.reserve(v.capacity());
out.resize(v.size());
memcpy(out.data(), v.data(), v.size() * sizeof(typename V::value_type));
return out;
}
template <typename V>
auto copy_mem(V const& v) {
KUL_DBG_FUNC_ENTER;
static_assert(std::is_trivially_copyable_v<typename V::value_type>,
"copy_mem can only be used with trivially copyable types");
V out;
out.reserve(v.capacity());
out.resize(v.size());
memcpy(out.data(), v.data(), v.size() * sizeof(typename V::value_type));
return out;
}


template <typename V>
auto copy_loop(V const& v) {
KUL_DBG_FUNC_ENTER;
V out;
out.reserve(v.capacity());
out.resize(v.size());
for (std::size_t i = 0; i < v.size(); ++i) out[i] = v[i];
// memcpy(out.data(), v.data(), v.size() * sizeof(typename V::value_type));
return out;
}

template <typename V>
auto make_vector(std::size_t const& size) {
KUL_DBG_FUNC_ENTER;
Expand All @@ -74,25 +96,40 @@ auto make_vector_from(V1 const& v1) {
return v;
}

template <typename V0>
void resize(V0& v) {
KUL_DBG_FUNC_ENTER;
v.resize(v.capacity() + 1); // force reallocation
}

template <typename T>
void do_compare() {
constexpr static std::size_t N = 2000000;
auto const std_vec = make_vector<std::vector<T>>(N);
auto const std_vec2 = make_vector_from<std::vector<T>>(std_vec);
auto const no_construct_vec = make_vector_from<mkn::kul::NonConstructingVector<T>>(std_vec);
if (std_vec != no_construct_vec) throw std::runtime_error("FAIL");
if (std_vec != std_vec2) throw std::runtime_error("FAIL");
auto const v0 = copy_construct(std_vec);
auto const v1 = copy_construct(no_construct_vec);
auto const v2 = copy_manual(std_vec);
auto const v3 = copy_manual(no_construct_vec);
auto const v4 = copy_operator_equal_super(no_construct_vec);

if (v0 != std_vec) throw std::runtime_error("FAIL 0");
if (v0 == v1) throw std::runtime_error("FAIL 1"); // :(
if (v0 != v2) throw std::runtime_error("FAIL 2");
if (v0 != v3) throw std::runtime_error("FAIL 3");
if (v0 != v4) throw std::runtime_error("FAIL 4");

auto wash = [&]<typename V>(auto const& name) {
KOUT(NON) << name;
auto const copy = make_vector_from<V>(v0);
auto const v1 = copy_mem(copy);
auto const v2 = copy_loop(copy);
auto const v3 = copy_manual(copy);

if (v0 != v1) throw std::runtime_error("FAIL 1"); // :(
if (v0 != v2) throw std::runtime_error("FAIL 2");
if (v0 != v3) throw std::runtime_error("FAIL 3");
};

using namespace mkn::kul;
wash.template operator()<std::vector<T>>("std::vector<T>");
wash.template operator()<NonConstructingVector<T>>("NonConstructingVector<T>");
wash.template operator()<HugePageVector<T>>("HugePageVector<T>");
wash.template operator()<NonConstructingHugePageVector<T>>("NonConstructingHugePageVector<T>");
}

TEST(NoConstructAllocator, copies) { do_compare<S<8>>(); }
TEST(NoConstructAllocator, copies) { do_compare<double>(); }
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add test coverage for non-trivial types.

While testing with double is good for verifying the basic functionality, consider adding test cases with non-trivial types to ensure proper behavior with complex objects.

 TEST(NoConstructAllocator, copies) { do_compare<double>(); }
+TEST(NoConstructAllocator, copies_complex) { do_compare<S<8>>(); }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
TEST(NoConstructAllocator, copies) { do_compare<double>(); }
TEST(NoConstructAllocator, copies) { do_compare<double>(); }
TEST(NoConstructAllocator, copies_complex) { do_compare<S<8>>(); }


// int main(int argc, char* argv[]) {
// ::testing::InitGoogleTest(&argc, argv);
// return RUN_ALL_TESTS();
// }
Loading