huge paging #60
huge paging #60
Conversation
WalkthroughThe pull request introduces significant modifications to several files, primarily enhancing memory allocation strategies through new allocator classes and adjustments to existing methods. Key additions include the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
mkn.yaml (1)
31-44: Consider removing redundant mode settingThe
mode: nonesetting appears in both_testandtestprofiles. Sincetestinherits from_test, this is redundant. Consider removing it from the child profile.Apply this diff to remove the redundancy:
- name: test parent: _test main: test/test.cpp src: test/test - mode: noneinc/mkn/kul/vector.hpp (4)
34-38: Remove duplicate include ofalloc.hppThere's a duplicate include of
alloc.hpp. One of these should be removed to avoid confusion.-#include "alloc.hpp" #include "mkn/kul/alloc.hpp"
42-43: Remove incorrect comment about parameter AThe comment refers to an ignored parameter
Abut the template only has parameterT. This appears to be outdated.-template <typename T> // A ignored but there for std::vector interop +template <typename T>
65-87: LGTM! Consider performance optimization for large vectorsThe comparison operators are well-implemented with proper type constraints and symmetry. However, for large vectors, consider using
std::equalfor potential performance benefits: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; + return std::equal(v0.begin(), v0.end(), v1.begin()); }
45-52: Add documentation for vector type specializationsConsider adding documentation comments for each vector type alias explaining:
- Their intended use cases
- Performance characteristics
- Memory allocation behavior
- When to use each type over the others
This will help users choose the appropriate vector type for their needs.
Example:
/** * Vector optimized for huge page allocations. * @tparam T The element type * @tparam S The huge page size in bytes (default: 4096) * * Use this when: * - Dealing with large datasets that benefit from huge pages * - Memory locality is critical for performance */ template <typename T, std::size_t S = 4096> using HugePageVector = std::vector<T, HugePageAllocator<T, S>>;test/test/no_construct_alloc.cpp (3)
75-84: Remove commented out code and consider extracting common setup.The commented out
memcpyline should be removed. Also, consider extracting the common reserve/resize pattern into a helper function to reduce code duplication.+template <typename V> +auto prepare_vector(V const& src) { + V out; + out.reserve(src.capacity()); + out.resize(src.size()); + 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()); + auto out = prepare_vector(v); 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; }
111-121: Enhance error messages and test coverage.The error messages "FAIL 1", "FAIL 2", etc. could be more descriptive. Also, consider testing with different vector sizes to ensure the copying methods work correctly across various scenarios.
- 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 != v1) throw std::runtime_error("memcpy copy failed to match original vector"); + if (v0 != v2) throw std::runtime_error("loop copy failed to match original vector"); + if (v0 != v3) throw std::runtime_error("manual copy failed to match original vector");
132-135: Remove commented-out main function.The commented-out main function appears to be unused debug code and should be removed for cleaner test file.
inc/mkn/kul/alloc.hpp (1)
163-163: Check the return value ofmadviseto handle potential errorsWhile
madviseis a performance hint and its failure may not be critical, checking its return value can help detect issues where the system does not support huge pages or when there are permission issues.Consider logging a warning if
madvisefails:+ if (madvise(p, n * sizeof(T), MADV_HUGEPAGE) != 0) { + // Handle or log the error appropriately + }Also applies to: 192-192
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
inc/mkn/kul/alloc.hpp(3 hunks)inc/mkn/kul/vector.hpp(2 hunks)mkn.yaml(2 hunks)test/test/no_construct_alloc.cpp(3 hunks)
🔇 Additional comments (7)
mkn.yaml (1)
1-1: Verify the impact of the new 'K' flag in build command
The build command has been modified to include a 'K' flag. Please ensure this change is documented and its purpose is clear to other developers.
Run the following script to check for documentation about the flag:
inc/mkn/kul/vector.hpp (1)
48-52: Verify huge page size constant
The default huge page size of 4096 bytes should be verified against system requirements and documentation. Consider making this value configurable or documenting why this specific size was chosen.
test/test/no_construct_alloc.cpp (2)
11-11: LGTM!
The addition of <cstring> header is necessary for the memcpy function used in the new copy_mem implementation.
99-103: LGTM!
The resize function correctly forces reallocation for testing purposes.
inc/mkn/kul/alloc.hpp (3)
40-42: Includes for memory management functions are appropriate
The added includes <limits>, <stdlib.h>, and <sys/mman.h> are necessary for the new allocator functionalities using std::numeric_limits, posix_memalign, and madvise.
105-105: Simplified allocate function enhances readability
Condensing the allocate method in the Allocator class into a single line improves code clarity without altering functionality.
134-137: Overriding construct methods appropriately
The construct methods are intentionally empty, which is appropriate for a non-constructing allocator. This ensures that object construction is skipped, aligning with the allocator's purpose.
test/test/no_construct_alloc.cpp
Outdated
| } | ||
|
|
||
| TEST(NoConstructAllocator, copies) { do_compare<S<8>>(); } | ||
| TEST(NoConstructAllocator, copies) { do_compare<double>(); } |
There was a problem hiding this comment.
🛠️ 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.
| TEST(NoConstructAllocator, copies) { do_compare<double>(); } | |
| TEST(NoConstructAllocator, copies) { do_compare<double>(); } | |
| TEST(NoConstructAllocator, copies_complex) { do_compare<S<8>>(); } |
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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; | |
| } |
inc/mkn/kul/alloc.hpp
Outdated
| template <typename T, std::size_t huge_page_size = 4096> | ||
| class HugePageAllocator : public Allocator<T> { |
There was a problem hiding this comment.
🛠️ 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
inc/mkn/kul/alloc.hpp
Outdated
| T* allocate(std::size_t const n) const { | ||
| // if (n == 0) return nullptr; | ||
| return static_cast<T*>(malloc(n * sizeof(T))); | ||
| } |
There was a problem hiding this comment.
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
mallocinstead of::operator newbypasses exception handling on allocation failure.mallocreturnsnullptron failure, which might not throw astd::bad_allocexception 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.
inc/mkn/kul/alloc.hpp
Outdated
| 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); | ||
| } |
There was a problem hiding this comment.
Correct error handling for posix_memalign and avoid potential undefined behavior
In the HugePageAllocator::allocate method:
posix_memalignreturns zero on success and a non-zero error code on failure. The value ofpis undefined if allocation fails. Checkingpfornullptris not reliable.madviseshould 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.
| 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); | |
| } |
inc/mkn/kul/alloc.hpp
Outdated
| 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); | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in NonConstructingHugePageAllocator::allocate
The same concerns apply here as in the HugePageAllocator:
- Check the return value of
posix_memaligninstead of theppointer. - Call
madviseonly 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.
| 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); | |
| } |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
inc/mkn/kul/alloc/huge.hpp (1)
49-50: Consider updating defaulthuge_page_sizeto match typical huge page sizesThe default
huge_page_sizeis set to4096bytes, which is the standard system page size. Huge pages are typically2MB(or2097152bytes) on many systems. Updating the default value can enhance the performance benefits of using huge pages.Apply this change to both allocator templates:
-template <typename T, std::size_t huge_page_size = 4096> +template <typename T, std::size_t huge_page_size = 2097152>Also applies to: 78-79
🛑 Comments failed to post (4)
inc/mkn/kul/alloc/huge.hpp (2)
92-99:
⚠️ Potential issueFix error handling in
allocatemethod by checkingposix_memalignreturn valueThe
allocatemethod inNonConstructingHugePageAllocatoralso lacks a check for the return value ofposix_memalign. This can result in undefined behavior if the allocation fails.Apply this diff to correct the error handling:
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(); + int ret = posix_memalign(&p, huge_page_size, n * sizeof(T)); + if (ret != 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.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 ret = posix_memalign(&p, huge_page_size, n * sizeof(T)); if (ret != 0) throw std::bad_alloc(); madvise(p, n * sizeof(T), MADV_HUGEPAGE); return static_cast<T*>(p); }
63-70:
⚠️ Potential issueFix error handling in
allocatemethod by checkingposix_memalignreturn valueThe
allocatemethod inHugePageAllocatordoes not check the return value ofposix_memalign. This omission can lead to undefined behavior if memory allocation fails becausepmay remain uninitialized, and subsequent usage ofpwould be invalid.Apply this diff to correctly handle allocation failures:
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(); + int ret = posix_memalign(&p, huge_page_size, n * sizeof(T)); + if (ret != 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.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 ret = posix_memalign(&p, huge_page_size, n * sizeof(T)); if (ret != 0) throw std::bad_alloc(); madvise(p, n * sizeof(T), MADV_HUGEPAGE); return static_cast<T*>(p); }inc/mkn/kul/alloc/base.hpp (2)
92-93: 🛠️ Refactor suggestion
Use
std::size_tfordiffto matchsizetype and avoid potential overflowIn
AlignedAllocator,diffis declared asstd::uint32_t, butsizeis of typestd::size_t. On systems wherestd::size_tis 64 bits, this can lead to incorrect calculations for large allocations.Apply this change to ensure type consistency:
- std::uint32_t diff = size % alignment; + std::size_t diff = size % alignment;📝 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.std::size_t diff = size % alignment; if (diff > 0) diff = alignment - diff;
125-128:
⚠️ Potential issueFix mismatch between allocation and deallocation functions in
NonConstructingAllocatorThe
allocatemethod usesmallocfor memory allocation, but thedeallocatemethod inherited fromAllocator<T>uses::operator deletefor deallocation. This mismatch can cause undefined behavior and memory corruption.Override the
deallocatemethods inNonConstructingAllocatorto usestd::free(p);:+ void deallocate(T* const p) noexcept { + if (p) std::free(p); + } + + void deallocate(T* const p, std::size_t /*n*/) noexcept { + deallocate(p); + }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
test/test/no_construct_alloc.cpp (4)
75-84: Remove commented out memcpy code.The function implements a safe element-by-element copy, making the commented memcpy line unnecessary.
Apply this diff:
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; }
99-103: Enhance documentation for resize function.The comment "force reallocation" could be more descriptive about the purpose and implications of this operation.
Add more detailed documentation:
template <typename V0> +/** + * Forces vector reallocation by increasing size beyond current capacity. + * Used for testing memory reallocation behavior of different allocator implementations. + */ void resize(V0& v) { KUL_DBG_FUNC_ENTER; v.resize(v.capacity() + 1); // force reallocation }
111-130: Consider parameterizing test sizes for better coverage.The test uses a lambda to test different vector implementations, which is good. However, consider:
- Testing with different sizes of
S<N>to verify behavior with varying object sizes- Testing edge cases (empty vectors, single element)
Example enhancement:
-TEST(NoConstructAllocator, copies) { do_compare<S<>>(); } +TEST(NoConstructAllocator, copies_small) { do_compare<S<4>>(); } +TEST(NoConstructAllocator, copies_medium) { do_compare<S<16>>(); } +TEST(NoConstructAllocator, copies_large) { do_compare<S<64>>(); } +TEST(NoConstructAllocator, copies_edge_cases) { + // Test with empty vector + constexpr static std::size_t N = 0; + auto const empty_vec = make_vector<std::vector<S<>>>(N); + // Add test cases for empty and single element vectors +}
135-138: Remove commented out main function.The commented out main function is unnecessary in a test file using Google Test.
Apply this diff:
-// int main(int argc, char* argv[]) { -// ::testing::InitGoogleTest(&argc, argv); -// return RUN_ALL_TESTS(); -// }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
inc/mkn/kul/alloc.hpp(1 hunks)inc/mkn/kul/alloc/base.hpp(1 hunks)inc/mkn/kul/alloc/huge.hpp(1 hunks)inc/mkn/kul/vector.hpp(2 hunks)test/test/no_construct_alloc.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- inc/mkn/kul/alloc/huge.hpp
- inc/mkn/kul/alloc/base.hpp
- inc/mkn/kul/vector.hpp
🔇 Additional comments (2)
inc/mkn/kul/alloc.hpp (1)
34-35: LGTM! Good modularization of allocator implementations.
The separation of allocator implementations into dedicated header files improves code organization and maintainability.
Let's verify the platform-specific availability of huge pages:
✅ Verification successful
Let me gather more information about the huge page implementation to verify its platform-specific nature:
Verified: Platform-specific huge page allocator is properly guarded
The code is well-structured with appropriate platform checks:
- The entire huge page implementation is guarded by
__has_include(<sys/mman.h>) - Additional runtime check for huge page support via
MADV_HUGEPAGEmacro - Clean fallback behavior - when not available, the header is effectively empty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for huge page support in the codebase
# Test: Look for platform-specific implementations and guards
rg -A 5 "__has_include\(<sys/mman\.h>\)"
Length of output: 1076
Script:
#!/bin/bash
# Check the implementation details of huge page allocator
cat inc/mkn/kul/alloc/huge.hpp
Length of output: 4210
test/test/no_construct_alloc.cpp (1)
65-73:
Add type trait check for safe memcpy usage.
Using memcpy is unsafe for non-trivially copyable types.
Add static assertion as previously suggested:
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;
}
PhilipDeegan
force-pushed
the
huge
branch
3 times, most recently
from
d58762e to
d47f5ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
inc/mkn/kul/alloc/aligned.hpp (1)
77-79: Consider documenting the unused size parameter.The size parameter in deallocate is unused but required by the standard allocator interface.
Add a comment explaining why the parameter is unused:
- void deallocate(T* const p, std::size_t /*n*/) noexcept { // needed from std:: + // Size parameter required by std::allocator but unused as aligned_alloc doesn't need it + void deallocate(T* const p, std::size_t /*n*/) noexcept {test/test/no_construct_alloc.cpp (2)
75-84: Remove commented out code and clarify implementation choice.The commented out memcpy suggests uncertainty about the implementation choice.
Remove the commented code and add a comment explaining the loop-based approach:
template <typename V> auto copy_loop(V const& v) { KUL_DBG_FUNC_ENTER; + // Using element-wise copy to support non-trivially copyable types 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; }
133-133: Add test coverage for different types and sizes.The test only covers S<> type. Consider adding more test cases.
Add tests for different types and sizes:
TEST(NoConstructAllocator, copies) { do_compare<S<>>(); } +TEST(NoConstructAllocator, copies_large) { do_compare<S<16>>(); } +TEST(NoConstructAllocator, copies_pod) { do_compare<int>(); } +TEST(NoConstructAllocator, copies_small) { do_compare<S<1>>(); }README.md (2)
5-5: Fix hyphenation in "Cross platform"The term should be hyphenated as "Cross-platform" according to standard English compound word rules.
-**Cross platform C++20 wrapper for threading/process management/io/logging/signal handling and stack walking.** +**Cross-platform C++20 wrapper for threading/process management/io/logging/signal handling and stack walking.**🧰 Tools
🪛 LanguageTool
[misspelling] ~5-~5: This word is normally spelled with a hyphen.
Context: # mkn.kul **Cross platform C++20 wrapper for threading/process man...(EN_COMPOUNDS_CROSS_PLATFORM)
5-5: Consider mentioning the huge paging allocator featureSince this project now includes a significant memory management feature with the huge paging allocator, consider adding this information to the description to highlight this capability.
-**Cross-platform C++20 wrapper for threading/process management/io/logging/signal handling and stack walking.** +**Cross-platform C++20 wrapper for threading/process management/io/logging/signal handling, stack walking, and huge page memory allocation.**🧰 Tools
🪛 LanguageTool
[misspelling] ~5-~5: This word is normally spelled with a hyphen.
Context: # mkn.kul **Cross platform C++20 wrapper for threading/process man...(EN_COMPOUNDS_CROSS_PLATFORM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.github/workflows/build_nix.yml(2 hunks).github/workflows/build_osx.yml(2 hunks).github/workflows/build_win.yml(2 hunks)README.md(1 hunks)inc/mkn/kul/alloc.hpp(1 hunks)inc/mkn/kul/alloc/aligned.hpp(1 hunks)inc/mkn/kul/alloc/base.hpp(1 hunks)inc/mkn/kul/alloc/huge.hpp(1 hunks)inc/mkn/kul/vector.hpp(2 hunks)mkn.yaml(2 hunks)test/test/no_construct_alloc.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- mkn.yaml
- inc/mkn/kul/alloc/huge.hpp
- inc/mkn/kul/vector.hpp
- inc/mkn/kul/alloc/base.hpp
🧰 Additional context used
🪛 LanguageTool
README.md
[misspelling] ~5-~5: This word is normally spelled with a hyphen.
Context: # mkn.kul **Cross platform C++20 wrapper for threading/process man...
(EN_COMPOUNDS_CROSS_PLATFORM)
🔇 Additional comments (6)
.github/workflows/build_nix.yml (2)
3-5: LGTM: Concurrency configuration is well-structured
The concurrency configuration will help optimize CI resources by canceling outdated workflow runs.
23-23: Verify C++20 requirement for huge paging allocator
The upgrade to C++20 suggests usage of newer language features. Let's verify which C++20 features are being utilized.
.github/workflows/build_osx.yml (1)
3-5: LGTM: Consistent with Linux workflow
The changes maintain parity with the Linux workflow configuration, ensuring consistent behavior across platforms.
Also applies to: 23-23
inc/mkn/kul/alloc.hpp (1)
34-36: LGTM! Good modular restructuring of allocator classes.
The separation of allocator implementations into distinct header files improves code organization and maintainability. The conditional includes with clear comments about availability are a good practice.
test/test/no_construct_alloc.cpp (2)
65-73:
Add type safety check for memcpy usage.
The copy_mem function uses memcpy which is only safe for trivially copyable types.
Add static assertion:
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;127-130: Add error handling for huge page allocation failures.
The huge page vector tests should handle potential allocation failures gracefully.
Let's verify if there's proper error handling in the huge page allocator:
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
💡 Codebase verification
Windows support missing in huge pages allocator implementation
The huge pages allocator implementation currently relies on POSIX-specific functions (posix_memalign and madvise) without Windows alternatives. This needs attention as:
- It uses
posix_memalignfor memory alignment - It uses
madvisewithMADV_HUGEPAGEfor huge pages support - No Windows-specific implementation using
VirtualAllocwithMEM_LARGE_PAGESis present
Required changes:
- Add Windows-specific implementation in
inc/mkn/kul/alloc/huge.hppusing Windows APIs - Implement proper conditional compilation for Windows vs POSIX systems
- Add Windows-specific tests for huge pages allocation
🔗 Analysis chain
LGTM: Windows-specific configuration is properly maintained
The changes correctly:
- Implement concurrency control matching other platforms
- Use the appropriate MSVC flag
-std:c++20for C++20 support - Maintain the required
-EHscflag for exception handling
Note: The huge paging allocator implementation should be tested thoroughly on Windows, as memory management features can have platform-specific behaviors.
Also applies to: 30-30
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Windows-specific memory management code
echo "Checking for Windows-specific huge pages implementation..."
# Search for Windows API usage related to large pages
rg "VirtualAlloc|GetLargePageMinimum|MEM_LARGE_PAGES"
# Search for conditional compilation for Windows
rg "#if(\s+|def\s+)(_WIN32|_MSC_VER)"
Length of output: 354
Script:
#!/bin/bash
# Let's examine the Windows-specific code in these files
echo "=== Windows-specific code in test/test/os.cpp ==="
rg -B 2 -A 5 "_WIN32" test/test/os.cpp
echo -e "\n=== Windows-specific code in inc/mkn/kul/os.hpp ==="
rg -B 2 -A 5 "_WIN32" inc/mkn/kul/os.hpp
# Look specifically for memory allocation related code
echo -e "\n=== Memory allocation related code ==="
rg -B 2 -A 5 "alloc|malloc|new\s+\w+\[|mmap" inc/mkn/kul/os.hpp test/test/os.cpp
# Check for any huge pages related implementation
echo -e "\n=== Huge pages related code ==="
rg -B 2 -A 5 "huge|large.*page|page.*size" inc/mkn/kul/os.hpp test/test/os.cpp
Length of output: 1588
Script:
#!/bin/bash
# Let's search for memory-related code in the entire codebase
echo "=== Searching for memory management implementation ==="
rg -B 2 -A 5 "class\s+.*Allocator|struct\s+.*Allocator"
echo -e "\n=== Searching for huge pages implementation ==="
fd . -e cpp -e hpp -e h --exec grep -l -i "huge.*page\|large.*page\|page.*size"
echo -e "\n=== Checking memory-related headers and implementations ==="
fd . -e cpp -e hpp -e h --exec grep -l "memory\|allocation\|allocator"
Length of output: 2967
Script:
#!/bin/bash
# Let's examine the huge pages implementation
echo "=== Checking huge pages implementation in inc/mkn/kul/alloc/huge.hpp ==="
rg -B 5 -A 10 "HugePageAllocator" inc/mkn/kul/alloc/huge.hpp
# Check for Windows-specific code in huge pages implementation
echo -e "\n=== Windows-specific code in huge pages implementation ==="
rg -B 2 -A 5 "_WIN32|_MSC_VER" inc/mkn/kul/alloc/huge.hpp
# Check test implementation
echo -e "\n=== Huge pages test implementation ==="
rg -B 2 -A 5 "huge|large.*page|page.*size" test/test/no_construct_alloc.cpp
Length of output: 2281
| template <typename T, std::int32_t alignment = 32> | ||
| class AlignedAllocator : public Allocator<T> { |
There was a problem hiding this comment.
Add alignment validation.
The alignment parameter should be validated to ensure it's a power of 2, as required by std::aligned_alloc.
Add this validation:
template <typename T, std::int32_t alignment = 32>
+static_assert(alignment > 0 && (alignment & (alignment - 1)) == 0,
+ "Alignment must be a positive power of 2");
class AlignedAllocator : public Allocator<T> {📝 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.
| template <typename T, std::int32_t alignment = 32> | |
| class AlignedAllocator : public Allocator<T> { | |
| template <typename T, std::int32_t alignment = 32> | |
| static_assert(alignment > 0 && (alignment & (alignment - 1)) == 0, | |
| "Alignment must be a positive power of 2"); | |
| class AlignedAllocator : public Allocator<T> { |
| auto size = n * sizeof(T); | ||
| std::uint32_t diff = size % alignment; | ||
| if (diff > 0) diff = alignment - diff; |
There was a problem hiding this comment.
Prevent potential overflow in alignment calculation.
The size and alignment calculations could overflow for large allocations.
Add overflow checks:
- auto size = n * sizeof(T);
- std::uint32_t diff = size % alignment;
- if (diff > 0) diff = alignment - diff;
+ if (n > std::numeric_limits<size_t>::max() / sizeof(T))
+ throw std::bad_alloc();
+ auto size = n * sizeof(T);
+ auto diff = size % alignment;
+ if (diff > 0) {
+ if (size > std::numeric_limits<size_t>::max() - (alignment - diff))
+ throw std::bad_alloc();
+ diff = alignment - diff;
+ }📝 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.
| auto size = n * sizeof(T); | |
| std::uint32_t diff = size % alignment; | |
| if (diff > 0) diff = alignment - diff; | |
| if (n > std::numeric_limits<size_t>::max() / sizeof(T)) | |
| throw std::bad_alloc(); | |
| auto size = n * sizeof(T); | |
| auto diff = size % alignment; | |
| if (diff > 0) { | |
| if (size > std::numeric_limits<size_t>::max() - (alignment - diff)) | |
| throw std::bad_alloc(); | |
| diff = alignment - diff; | |
| } |
Summary by CodeRabbit
Release Notes
New Features
HugePageAllocator,NonConstructingHugePageAllocator, andAlignedAllocator.NonConstructingHugePageVectorandHugePageVector.Bug Fixes
Documentation
Tests