Skip to content

Select mmap madvise by index type: MADV_RANDOM for DiskSeismic, not a global hugepage default - #55

Merged
chishui merged 1 commit into
opensearch-project:mainfrom
zirui-song-18:mmap-advise-per-index-type
Sep 14, 2026
Merged

chishui merged 1 commit into
opensearch-project:mainfrom
zirui-song-18:mmap-advise-per-index-type

Conversation

@zirui-song-18

Copy link
Copy Markdown
Collaborator

Description

What

MmapFile applied one global madvise default — MADV_HUGEPAGE — to every index type. That is right for the shared Seismic indexes (queries scan broad regions of one forward index), but wrong for DiskSeismic / per_block, whose whole design is to point-read only ~k′ small (~8 KiB) inline-forward blocks per query. Hugepage (2 MiB fault granularity) plus RAID readahead turns each of those tiny reads into a huge one.

This makes the madvise hint a function of the index's access pattern instead of a single global default:

  • DiskSeismic (DiskSeismicIndex, DiskSeismicScalarQuantizedIndex) → MADV_RANDOM (point lookups; suppress readahead).
  • shared (SeismicIndex, SeismicScalarQuantizedIndex) → MADV_HUGEPAGE (broad scans; fewer TLB entries), unchanged.

NSPARSE_MMAP_ADVISE still overrides the choice (an empty value is treated as unset, so it can't silently reinstate the old default).

Measured impact

From the launch benchmark (i4i.16xlarge, MS MARCO V1, 12 GB page-cache cap, THP=[madvise], top_n=5, k′=20, recall ≈ 0.92), same index and queries:

arm advise p50 p99
disk (per_block) hugepage (old default) 6.529 ms 13.21 ms
disk (per_block) random (new default) 0.157 ms 0.296 ms
  • ~42× lower p50, ~45× lower p99. The old default was reading ~9,426 KiB per query for the ~53–1,055 KiB a query actually needs (~860× amplification over a 4 KiB page once RAID readahead compounds it). latency (0.157 ms) — the cap costs it nothing, using < 4.76 GB of page cache.
  • Resident memory for the disk arm drops from > 30.9 GB to 10.94 GB (−65%); the previously reported "per_block needs ~3× more RAM than shared" was largely a hugepage artifact (real ratio ~1.6×).

Implementation notes

  • The choice lives in a single pure helper (MmapFile::resolve_advise) that is unit-tested, and DiskSeismic types map their file through one base-class entry point (DiskSeismicIndexBase::map_index_file) so a future disk index type can't silently fall back to the scan/hugepage default.

Testing

  • All unit tests pass (654), including new coverage for the per-type default, the env override, empty-env-as-unset, and reading identical bytes under either pattern.
  • Verified end to end with strace: a DiskSeismic serialized-index mapping issues MADV_RANDOM, a shared mapping issues MADV_HUGEPAGE.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…ion based on index type

Signed-off-by: Zirui Song <zrsong@amazon.com>
@chishui

chishui commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

the PR looks good to me, but before approval, it's better to root cause the reason of degradation first

@chishui

chishui commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

discussed offline, this is a bug not a degradation and it impacts disk seismic's index performance when memory is not enough

Comment thread nsparse/utils/mmap_file.h
}

const char* advise = std::getenv("NSPARSE_MMAP_ADVISE");
std::string mode = (advise != nullptr) ? advise : "hugepage";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

previous logic is: for all indices, no madvise is called, now you change to only set disk seismic index to random

@chishui
chishui merged commit 2da4627 into opensearch-project:main Sep 14, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants