Skip to content

perf(search): share a thread-local RNG for HNSW and drop seed (#2398) #3562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
advisedy wants to merge 8 commits into
base: unstable
Choose a base branch
from
+11 −13
Open

perf(search): share a thread-local RNG for HNSW and drop seed (#2398) #3562

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3e3b1aa
perf(search): lazily initialize HNSW random generator
advisedy Jul 20, 2026
5cfd455
fix(search): only call random_device when seed is not set
advisedy Jul 20, 2026
6c3ad7d
Merge branch 'unstable' into #2398
advisedy Jul 20, 2026
d53751b
Merge branch 'unstable' into #2398
jihuayu Jul 21, 2026
3b7a688
perf(search): use a static thread-local RNG for HNSW indexes
advisedy Jul 23, 2026
ff60ae2
perf(search): drop seed, make the RNG a class member
advisedy Jul 23, 2026
859bbaf
Merge branch 'unstable' into #2398
advisedy Jul 23, 2026
03c941f
fix(search): mark RandomizeLayer and InsertVectorEntry as const
advisedy Jul 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/search/hnsw_indexer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@

#include <algorithm>
#include <cmath>
#include <memory>
#include <queue>
#include <random>
#include <unordered_set>
Expand Down Expand Up @@ -173,14 +172,15 @@ StatusOr<double> ComputeSimilarity(const VectorItem& left, const VectorItem& rig
}

HnswIndex::HnswIndex(const SearchKey& search_key, HnswVectorFieldMetadata* vector, engine::Storage* storage,
std::random_device::result_type seed)
std::optional<std::random_device::result_type> seed)
: search_key(search_key),
metadata(vector),
storage(storage),
generator(std::mt19937(seed)),
seed(seed),
m_level_normalization_factor(1.0 / std::log(metadata->m)) {}

uint16_t HnswIndex::RandomizeLayer() {
static thread_local std::mt19937 generator = [this] { return std::mt19937(seed.value_or(std::random_device()())); }();

@PragmaTwice PragmaTwice Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. Please don't make it a local static variable;
  2. The semantics of seed is very weird here.

@advisedy advisedy Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right — the semantics of seed is indeed weird here. Since the
generator is now shared, only the first instance's seed would take
effect, which is misleading.

I see two possible directions:

  1. Share the generator only among indexes with the same seed, and let
    indexes with different seeds keep their own. I haven't fully figured
    out the implementation yet, but maybe a static thread_local map
    could work? It seems like that might be doable.

  2. Drop seed entirely, seed the generator with std::random_device,
    and make it a class-level static thread_local std::mt19937 member
    (no longer a function-local static), so its initialization no longer
    depends on any specific instance.

I'd like to know which direction you think we should take. If you
prefer option 1, I'll think about how to implement it in detail. If
it's option 2, I'll go ahead and do it now.

@PragmaTwice PragmaTwice Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you stop copy-pasting to me what your agent replies to you? You can do your own decision. I have enough token that I don't need an agent proxy.

@advisedy advisedy Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, my english is not good, so I tell AI what I was thinking and let it translate. But The decision is mine. please.

@advisedy advisedy Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is my origin words: 确实感觉seed有点奇怪在这里，但是generator得根据seed去生成出来，不如说这里seed，generator，和我们期望共享的逻辑有点混乱，我的理解有两种思路，第一种思路就是我们考虑对于seed一样的进行共享，不一样的就是各自持有，这种方法用static + thread_local + map 应该能实现出来？ 但我感觉有点麻烦。第二种思路就是或许可以考虑直接把seed删掉，使用std::random_device，这样的话，static thread_local std::mt19937 generator; 放到类里面，不再是局部了，这样初始化也不会依赖某个实例。 想知道一下，你觉得我们应该怎么做会比较好呢？如果你想要第一种的思路，我再去思考具体该怎么实现会比较好。如果是第二种的话，我可以现在就去实现出来代码。

@advisedy advisedy Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I’m very sorry that I relied too much on AI when replying, but my genuine intention was to contribute. I’m truly sorry.

@PragmaTwice PragmaTwice Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. I think we can just remove seed here.
  2. After the change, don't forget to rewrite the PR title and description to match the current implementation.

std::uniform_real_distribution<double> level_dist(0.0, 1.0);
double r = level_dist(generator);
double log_val = -std::log(r);
Expand Down
5 changes: 3 additions & 2 deletions src/search/hnsw_indexer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#pragma once

#include <optional>
#include <random>
#include <string>
#include <vector>
Expand Down Expand Up @@ -89,11 +90,11 @@ struct HnswIndex {
HnswVectorFieldMetadata* metadata;
engine::Storage* storage = nullptr;

std::mt19937 generator;
std::optional<std::random_device::result_type> seed;
double m_level_normalization_factor;

HnswIndex(const SearchKey& search_key, HnswVectorFieldMetadata* vector, engine::Storage* storage,
std::random_device::result_type seed = std::random_device()());
std::optional<std::random_device::result_type> seed = std::nullopt);

static StatusOr<std::vector<VectorItem>> DecodeNodesToVectorItems(engine::Context& ctx,
const std::vector<NodeKey>& node_key,
Expand Down
Loading