perf(search): share a thread-local RNG for HNSW and drop seed (#2398) by advisedy · Pull Request #3562 · apache/kvrocks

advisedy · 2026-07-20T00:41:46Z

Problem

HnswIndex currently owns a std::mt19937 generator directly, so every
HnswIndex construction also constructs the random generator.

However, the generator is only needed by RandomizeLayer() when inserting
vector entries. HnswIndex can also be constructed on query paths such as
KNN/RANGE search, where random layer generation is not used.

Fix

Replace the eager std::mt19937 member with a lazily initialized
std::unique_ptr<std::mt19937>.
Initialize the generator only on the first RandomizeLayer() call.

Tests

new C++ gtests added (hnsw_index_test.cc):

RandomGeneratorIsInitializedLazily

AI-assisted contribution

I used an LLM to help draft the new GTest case for verifying lazy generator
initialization.

Copilot

Pull request overview

This PR optimizes redis::HnswIndex construction cost on query paths by avoiding eager construction of a large std::mt19937 member, and instead initializing the RNG only when layer randomization is actually needed during inserts.

Changes:

Replace the eager std::mt19937 member with a lazily initialized std::unique_ptr<std::mt19937> plus an optional stored seed.
Initialize the RNG on first RandomizeLayer() call rather than in the constructor.
Add a new C++ gtest that asserts the RNG is not initialized until RandomizeLayer() is invoked.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`src/search/hnsw_indexer.h`	Changes `HnswIndex` RNG-related members/signature to support lazy initialization.
`src/search/hnsw_indexer.cc`	Implements lazy RNG allocation/initialization inside `RandomizeLayer()`.
`tests/cppunit/hnsw_index_test.cc`	Adds a regression test to verify RNG is initialized lazily.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

sonarqubecloud · 2026-07-20T11:30:50Z

Quality Gate failed

Failed conditions
38.3% Coverage on New Code (required ≥ 50%)
D Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

💡 Need a hand with PR review? Try Gitar by Sonar!

jihuayu

Thanks! LGTM

advisedy · 2026-07-23T03:25:29Z

Hi, @git-hulk , @PragmaTwice ,just friendly ping on this one too, whenever you have some time. Thanks!

git-hulk · 2026-07-23T10:55:21Z

 uint16_t HnswIndex::RandomizeLayer() {
+  if (!generator) {
+    const auto actual_seed = seed ? *seed : std::random_device()();
+    generator = std::make_unique<std::mt19937>(actual_seed);


From the original issue, it should expect to keep only one random generator for all HNSW indexes? @PragmaTwice

Yup. maybe thread local to avoid race.

switched to a single static thread_local generator as you suggested. please take a look.

PragmaTwice

Refer to the issue, I think it's better to make the RNG static.

advisedy · 2026-07-23T13:16:59Z

I was being silly and misunderstood，Now fixed: switched to a single static thread_local generator as you suggested. Updated in the latest push, please take a look @PragmaTwice .

PragmaTwice · 2026-07-23T13:49:21Z

+                     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()())); }();


Please don't make it a local static variable;

The semantics of seed is very weird here.

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:

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.

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.

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.

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

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; 放到类里面，不再是局部了，这样初始化也不会依赖某个实例。想知道一下，你觉得我们应该怎么做会比较好呢？如果你想要第一种的思路，我再去思考具体该怎么实现会比较好。如果是第二种的话，我可以现在就去实现出来代码。

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

I think we can just remove seed here.

After the change, don't forget to rewrite the PR title and description to match the current implementation.

jihuayu · 2026-07-24T01:00:16Z

Hi @advisedy.

There is nothing wrong with using AI to translate your writing into English, but do not let it turn a simple point into something unnecessarily long and bloated.

No one wants to feel as though they are talking to an AI agent. Copying and pasting an AI-generated response without editing it shows little respect for other people’s time. You need to think through what the AI gives you, incorporate your own understanding, and express it in your own words.

We are not short on tokens, but we do not need robots either. We are not looking for contributors who are only here to meet a KPI. We want contributors to grow with the community and remain part of it over the long term. That requires you to spend real time and effort thinking about and understanding the project, rather than relying entirely on AI to complete your tasks.

It is okay if you do not do everything well at the beginning, so there is no need to worry too much. I hope you can improve over time.

PragmaTwice

The code is good now. To get merged you need to check why the lint failed and fix it (just to add a const specifier).

perf(search): lazily initialize HNSW random generator

3e3b1aa

jihuayu requested review from Beihao-Zhou and Copilot July 20, 2026 09:03

Copilot started reviewing on behalf of jihuayu July 20, 2026 09:04 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

Comment thread src/search/hnsw_indexer.cc Outdated

advisedy and others added 2 commits July 20, 2026 17:22

fix(search): only call random_device when seed is not set

5cfd455

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Merge branch 'unstable' into apache#2398

6c3ad7d

jihuayu previously approved these changes Jul 21, 2026

View reviewed changes

jihuayu requested review from PragmaTwice and git-hulk July 21, 2026 01:09

Merge branch 'unstable' into apache#2398

d53751b

git-hulk reviewed Jul 23, 2026

View reviewed changes

PragmaTwice requested changes Jul 23, 2026

View reviewed changes

perf(search): use a static thread-local RNG for HNSW indexes

3b7a688

advisedy dismissed jihuayu’s stale review via 3b7a688 July 23, 2026 13:10

advisedy requested a review from PragmaTwice July 23, 2026 13:17

git-hulk requested a review from jihuayu July 23, 2026 13:41

PragmaTwice reviewed Jul 23, 2026

View reviewed changes

perf(search): drop seed, make the RNG a class member

ff60ae2

advisedy changed the title ~~perf(search): lazily initialize HNSW random generator (#2398)~~ perf(search): share a thread-local RNG for HNSW and drop seed (#2398) Jul 23, 2026

Merge branch 'unstable' into apache#2398

859bbaf

advisedy requested a review from PragmaTwice July 23, 2026 16:57

PragmaTwice reviewed Jul 24, 2026

View reviewed changes

Uh oh!

Conversation

advisedy commented Jul 20, 2026

Problem

Fix

Tests

AI-assisted contribution

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

sonarqubecloud Bot commented Jul 20, 2026

Quality Gate failed

Uh oh!

jihuayu left a comment

Choose a reason for hiding this comment

Uh oh!

advisedy commented Jul 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PragmaTwice left a comment

Choose a reason for hiding this comment

Uh oh!

advisedy commented Jul 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihuayu commented Jul 24, 2026

Uh oh!

PragmaTwice left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PragmaTwice left a comment •

edited

Loading