Async mmap population#36668
Conversation
387c908 to
ec1fca8
Compare
| * 0). | ||
| */ | ||
| void vm_prefetch(void* ptr, size_t size, size_t num_threads = 0) noexcept; | ||
| PrefetchToken vm_prefetch_async(void* ptr, size_t size) noexcept; |
There was a problem hiding this comment.
Old function should be preserved.
vm_prefetch_async with the same input parameters.
Is possible to hide the token from user (use void) and fire the fetch in background?
| * mapping when set to auto_size. | ||
| * @return A @ref util::PrefetchToken owning any spawned worker threads (empty by default). | ||
| */ | ||
| virtual util::PrefetchToken hint_prefetch_async(size_t offset = 0, size_t size = auto_size) { |
There was a problem hiding this comment.
Skip it for now lets focus on util that will work then we decide how it work.
If kept make pure virtual
| } | ||
|
|
||
| void vm_prefetch(void* ptr, size_t size, size_t num_threads) noexcept { | ||
| void vm_prefetch(void* ptr, size_t size, bool fast) noexcept { |
| } | ||
|
|
||
| void hint_prefetch_async(size_t offset, size_t size) override { | ||
| if (const auto plan = util::make_prefetch_plan(m_data, m_size, offset, size); plan.m_aligned_size) { |
There was a problem hiding this comment.
This change is not required old function can be use just set caculated region into new fetch function
| * Starts populating the given region of the mapping in background threads and returns | ||
| * immediately. Unlike the lower-level @ref util::vm_prefetch_async, no token is handed back | ||
| * to the caller: the implementation is fully responsible for tracking the background work | ||
| * internally and joining it before this object is destroyed (so the mapping is never torn | ||
| * down while a background task might still be touching it), regardless of what the caller | ||
| * does afterwards. This avoids requiring the caller to correctly manage a token's lifetime | ||
| * relative to this object's lifetime, which is an easy way to introduce a use-after-free. |
There was a problem hiding this comment.
The comment should be updated (make it shorter) information of token etc is not required. It should give information that start fetch data in async way.
| #include <sstream> | ||
| #include <thread> | ||
| #include <utility> | ||
|
|
There was a problem hiding this comment.
I can see that you added a new "compute" scenario test to FileLoadBenchmark pull/36741
Do you plan to extend FileLoadBenchmark with hint_prefetech_async in this pr?
ec1fca8 to
06f279d
Compare
38a3431 to
5677bb3
Compare
6c189f2 to
29d58bc
Compare
29d58bc to
ab5e330
Compare
| * @brief Number of parallel chunks a @p size byte job should be split into: at least one, at most | ||
| * @p max_chunks, targeting roughly one chunk per @p min_chunk bytes. | ||
| */ | ||
| constexpr size_t split_chunk_count(size_t size, size_t min_chunk, size_t max_chunks) noexcept { |
There was a problem hiding this comment.
Lets keep as internal util function.
| } | ||
|
|
||
| void vm_decommit(void* ptr, size_t size) noexcept { | ||
| assert(ptr != nullptr && size > 0); |
| } | ||
|
|
||
| void vm_release(void* ptr, size_t size) noexcept { | ||
| assert(ptr != nullptr && size > 0); |
| } else { | ||
| // blocks until every page has been faulted in. | ||
| populate_pages(ptr, size, num_threads); | ||
| return; |
| } | ||
|
|
||
| void vm_prefetch(void* ptr, size_t size, size_t num_threads) noexcept { | ||
| assert(ptr != nullptr && size > 0); |
|
|
||
| void hint_evict(size_t offset, size_t size) noexcept override { | ||
| if (m_mapped_view != MAP_FAILED) { | ||
| if (const auto region = util::make_madvise_region(m_data, m_size, offset, size); region.m_length > 0) { |
There was a problem hiding this comment.
Why changed? this is not part of this PR
| * @param size The size of the region. | ||
| * @return AlignedRegion The aligned memory region. | ||
| */ | ||
| inline util::AlignedRegion make_madvise_region(const void* data, size_t mapping_size, size_t offset, size_t size) { |
There was a problem hiding this comment.
This is not part of this PR
|
|
||
| void hint_prefetch(size_t offset, size_t size) override; | ||
|
|
||
| void hint_prefetch_async(size_t /*offset*/, size_t /*size*/) override {} |
There was a problem hiding this comment.
Why there is no windows implementration?
| endif() | ||
|
|
||
| target_include_directories(${TARGET_NAME} PUBLIC $<BUILD_INTERFACE:${UTIL_INCLUDE_DIR}>) | ||
| target_include_directories(${TARGET_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src) |
There was a problem hiding this comment.
Detail:
Just expand line above with PRIVATE section instead add target_include_directories new entry
| const size_t hw_threads = (std::max)(size_t{1}, static_cast<size_t>(std::thread::hardware_concurrency())); | ||
| const size_t max_by_size = size / (1024 * 1024); // 1 thread per MB | ||
| const size_t num_threads = (std::max)(size_t{1}, (std::min)(hw_threads, max_by_size)); | ||
| const size_t num_threads = split_chunk_count(size, one_mib, hw_threads); // 1 thread per MB, capped by HW |
There was a problem hiding this comment.
Lets not touch this source focus on mmap population only
Details:
Tickets:
AI Assistance: