diff --git a/nsparse/disk_seismic_index.cpp b/nsparse/disk_seismic_index.cpp index e375cc4..a5cffd2 100644 --- a/nsparse/disk_seismic_index.cpp +++ b/nsparse/disk_seismic_index.cpp @@ -53,7 +53,7 @@ DiskSeismicIndex* DiskSeismicIndex::mmap_index(const IndexHeader& header, throw_if_null(index_file, "index_file must not be null"); auto index = std::make_unique(header.dimension); - MmapFile mmap_file(std::string{index_file}); + MmapFile mmap_file = map_index_file(index_file); MmapCursor cursor(mmap_file.data(), mmap_file.size()); cursor.skip(pos); diff --git a/nsparse/disk_seismic_index_base.h b/nsparse/disk_seismic_index_base.h index 6a44693..3f9bf5a 100644 --- a/nsparse/disk_seismic_index_base.h +++ b/nsparse/disk_seismic_index_base.h @@ -12,6 +12,7 @@ #include #include +#include #include #include "nsparse/cluster/inverted_list_clusters.h" @@ -117,6 +118,15 @@ class DiskSeismicIndexBase : public MmapIndex, public IndexIO { // reads its own extra header first, then calls this. void load_mapped_payload(MmapCursor* cursor, MmapFile&& mapped); + // Maps a disk index file for querying. Every disk index is point-accessed + // (~k' small inline-forward blocks per query), so the mapping must use + // MADV_RANDOM. Concrete mmap_index() factories map through here so a new + // disk type cannot silently fall back to the scan/hugepage default. + static MmapFile map_index_file(const char* index_file) { + return MmapFile(std::string{index_file}, + MmapFile::AccessPattern::kPointLookup); + } + // Borrowed from by score_summaries_transposed / the inline forward index, so // the concrete validate_mapped_payload can inspect their widths. std::vector clustered_inverted_lists; diff --git a/nsparse/disk_seismic_scalar_quantized_index.cpp b/nsparse/disk_seismic_scalar_quantized_index.cpp index 48040a9..80c13e4 100644 --- a/nsparse/disk_seismic_scalar_quantized_index.cpp +++ b/nsparse/disk_seismic_scalar_quantized_index.cpp @@ -143,7 +143,7 @@ DiskSeismicScalarQuantizedIndex* DiskSeismicScalarQuantizedIndex::mmap_index( auto index = std::make_unique(header.dimension); - MmapFile mmap_file(std::string{index_file}); + MmapFile mmap_file = map_index_file(index_file); MmapCursor cursor(mmap_file.data(), mmap_file.size()); cursor.skip(pos); diff --git a/nsparse/utils/mmap_file.h b/nsparse/utils/mmap_file.h index 2fb4314..b7d6f8c 100644 --- a/nsparse/utils/mmap_file.h +++ b/nsparse/utils/mmap_file.h @@ -49,8 +49,31 @@ namespace nsparse { // point their data structures directly into the returned bytes. class MmapFile { public: + // How the region is read, which picks the default madvise hint below. + // kScan expects broad reads over one forward index (MADV_HUGEPAGE, fewer + // TLB entries); kPointLookup expects scattered small reads (MADV_RANDOM, no + // readahead) -- what DiskSeismic's ~k' inline blocks per query need. + // NSPARSE_MMAP_ADVISE overrides this. + enum class AccessPattern { kScan, kPointLookup }; + + // The madvise mode for `access`, or `env` verbatim when it is non-null + // (NSPARSE_MMAP_ADVISE overrides the per-type default). Pure so the choice + // is unit-testable without a real mapping. + static std::string resolve_advise(AccessPattern access, const char* env) { + // An empty NSPARSE_MMAP_ADVISE (set but "") is treated as unset, so it + // does not silently override the per-type default with the "hugepage" + // fallback below. + if (env != nullptr && *env != '\0') { + return std::string(env); + } + return access == AccessPattern::kPointLookup ? "random" : "hugepage"; + } + MmapFile() = default; - explicit MmapFile(const std::string& path) { open(path); } + explicit MmapFile(const std::string& path, + AccessPattern access = AccessPattern::kScan) { + open(path, access); + } ~MmapFile() { close(); } MmapFile(const MmapFile&) = delete; @@ -67,7 +90,8 @@ class MmapFile { const uint8_t* data() const { return data_; } size_t size() const { return size_; } - void open(const std::string& path) { + void open(const std::string& path, + AccessPattern access = AccessPattern::kScan) { close(); #if defined(_WIN32) file_ = CreateFileA(path.c_str(), GENERIC_READ, FILE_SHARE_READ, nullptr, @@ -116,8 +140,11 @@ class MmapFile { return; // empty file: leave data_ null } - const char* advise = std::getenv("NSPARSE_MMAP_ADVISE"); - std::string mode = (advise != nullptr) ? advise : "hugepage"; + // NSPARSE_MMAP_ADVISE overrides everything; otherwise the access + // pattern picks the default: point lookups (DiskSeismic) suppress + // readahead (MADV_RANDOM), scans collapse to huge pages. + const std::string mode = + resolve_advise(access, std::getenv("NSPARSE_MMAP_ADVISE")); // "hugetlb": copy the file into an anonymous MAP_HUGETLB region so the // index data is backed by real, pre-reserved 2 MiB pages (vm.nr_hugepages @@ -172,9 +199,9 @@ class MmapFile { // - MADV_HUGEPAGE lets the kernel back the region with 2 MiB pages, // cutting TLB entries ~512x on the random-access dot-product path, // at the cost of readahead the huge pages imply. - // Which wins is size-dependent, so the hint is selectable at runtime via - // NSPARSE_MMAP_ADVISE = "hugepage" | "random" | "normal" | "hugetlb" - // (default: hugepage, best for the large indexes this class targets). + // Which wins depends on the access pattern (see AccessPattern), so the + // default is per index type; NSPARSE_MMAP_ADVISE overrides it with + // "hugepage" | "random" | "normal" | "hugetlb". if (mode == "random") { ::madvise(addr, size_, MADV_RANDOM); } else if (mode == "normal") { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8fc3cd8..cfcbb45 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -24,6 +24,7 @@ set(NSPARSE_TEST_SRC inverted_lists_test.cpp kmeans_utils_test.cpp mmap_cursor_test.cpp + mmap_file_test.cpp mmap_index_test.cpp offset_width_test.cpp prefetch_test.cpp diff --git a/tests/mmap_file_test.cpp b/tests/mmap_file_test.cpp new file mode 100644 index 0000000..35f7fff --- /dev/null +++ b/tests/mmap_file_test.cpp @@ -0,0 +1,82 @@ +/** + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +#include "nsparse/utils/mmap_file.h" + +#include + +#include +#include +#include +#include +#include +#include + +namespace nsparse { +namespace { + +using AccessPattern = MmapFile::AccessPattern; + +// The per-type default: DiskSeismic (point lookups of ~k' small inline blocks) +// must suppress readahead with MADV_RANDOM, while the scanned shared forward +// index collapses to huge pages. A regression here silently reinstates the +// ~860x read amplification that makes per_block look slower than shared. +TEST(MmapFileAdvise, DefaultsByAccessPattern) { + EXPECT_EQ(MmapFile::resolve_advise(AccessPattern::kPointLookup, nullptr), + "random"); + EXPECT_EQ(MmapFile::resolve_advise(AccessPattern::kScan, nullptr), + "hugepage"); +} + +// NSPARSE_MMAP_ADVISE overrides the per-type default, for either pattern. +TEST(MmapFileAdvise, EnvOverridesEitherPattern) { + for (const char* env : {"hugepage", "random", "normal", "hugetlb"}) { + EXPECT_EQ(MmapFile::resolve_advise(AccessPattern::kPointLookup, env), + env); + EXPECT_EQ(MmapFile::resolve_advise(AccessPattern::kScan, env), env); + } +} + +// An empty env var (set but "") is treated as unset, not as an override that +// would silently force the "hugepage" fallback and undo DiskSeismic's default. +TEST(MmapFileAdvise, EmptyEnvIsTreatedAsUnset) { + EXPECT_EQ(MmapFile::resolve_advise(AccessPattern::kPointLookup, ""), + "random"); + EXPECT_EQ(MmapFile::resolve_advise(AccessPattern::kScan, ""), "hugepage"); +} + +// The access-pattern hint is advisory: the mapping returns the file's bytes +// unchanged under either pattern. +TEST(MmapFileAdvise, MapsSameBytesUnderEitherPattern) { + const std::filesystem::path path = + std::filesystem::temp_directory_path() / "nsparse_mmap_advise_test.bin"; + std::vector payload(8192); + for (size_t i = 0; i < payload.size(); ++i) { + payload[i] = static_cast(i * 7 + 1); + } + { + std::ofstream out(path, std::ios::binary | std::ios::trunc); + out.write(reinterpret_cast(payload.data()), + static_cast(payload.size())); + } + + for (const AccessPattern access : + {AccessPattern::kPointLookup, AccessPattern::kScan}) { + MmapFile file(path.string(), access); + ASSERT_EQ(file.size(), payload.size()); + ASSERT_NE(file.data(), nullptr); + EXPECT_EQ(std::memcmp(file.data(), payload.data(), payload.size()), 0); + } + + std::error_code ignored; + std::filesystem::remove(path, ignored); +} + +} // namespace +} // namespace nsparse