Add a better page cache dropping function for per file, unprivileged use #925
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
kingcrimsontianyu
changed the title
kingcrimsontianyu
added
c++
|
|
||
| // 6 pages remain in the page cache | ||
| auto [cached_pages, _] = kvikio::get_page_cache_info(_filepath.string()); | ||
| EXPECT_EQ(cached_pages, 6); |
There was a problem hiding this comment.
The test file only occupies 10 pages, and on a normally operating CI, I'd expect the file data to be fully resident in the page cache. In an extreme case when the CI system receives complete memory pressure, it is possible, on paper, that some pages of this test file get evicted, where the condition should be EXPECT_LE(cached_pages, 6); but I consider that case extremely unlikely.
I'm open to changing this to LE for absolute correctness though.
There was a problem hiding this comment.
let's see if this causes issues in CI :)
| drop_file_page_cache(file.fd(), offset, length, sync_first); | ||
| } | ||
|
|
||
| bool drop_system_page_cache(bool reclaim_dentries_and_inodes, bool sync_first) |
There was a problem hiding this comment.
Notes: drop_system_page_cache 's return value reflects whether the users have the elevated privilege to drop the system wide cache using /proc/sys/vm/drop_caches, whereas drop_file_page_cache has no privilege requirement. Hence the return value asymmetry.
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
small suggestion on the error APIs, the actual meat of the PR looks good.
cpp/src/error.cpp
Outdated
| GenericSystemError::GenericSystemError(const std::string& msg) : GenericSystemError(msg.c_str()) {} | ||
|
|
||
| GenericSystemError::GenericSystemError(const char* msg) : GenericSystemError(errno, msg) {} |
There was a problem hiding this comment.
can we avoid these overloads with std::string_view?
There was a problem hiding this comment.
Thanks. I noticed that the base class std::system_error has the following overloads:
system_error( std::error_code ec, const std::string& what_arg );
system_error( std::error_code ec, const char* what_arg );
Since the base class has no overload with string_view, the subclass, if having a string_view in the constructor, needs to pass it to the base class's std::string overload (performs string construction, defying micro-optimization), or to char const* overload (this looks dangerous since string_view.data() is likely not null terminated).
But I think the existing code GenericSystemError::GenericSystemError(const std::string& msg) : GenericSystemError(msg.c_str()) {} defies the micro-optimization too, as a string is constructed from the char array for the base class. So I've just made some further changes.
There was a problem hiding this comment.
So with how GenericSystemError(std::string const& msg); and GenericSystemError(char const* msg); are defined now, I think copy is completely avoided.
|
|
||
| // 6 pages remain in the page cache | ||
| auto [cached_pages, _] = kvikio::get_page_cache_info(_filepath.string()); | ||
| EXPECT_EQ(cached_pages, 6); |
There was a problem hiding this comment.
let's see if this causes issues in CI :)
|
/merge |
3 participants
This PR adds a new utility function
drop_file_page_cache()which usesposix_fadvise(POSIX_FADV_DONTNEED)toevict cached pages for a specific file. Unlike the existing system-wide cache drop, this approach has the following advantages:
This is the preferred method for benchmark cache invalidation, which is inspired by
fioimplementation.This PR also renames
clear_page_cache()todrop_system_page_cache()to align with Linux kernel terminology. The old function is now deprecated.