Add sq for hnsw #311

hhy3 · 2023-12-19T02:59:45Z

issue: #312

mergify · 2023-12-19T03:00:24Z

@hhy3 🔍 Important: PR Classification Needed!

For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:

If you're fixing a bug, label it as kind/bug.
For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
Adjusting APIs or changing functionality? Go with kind/feature.

For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”.

Thanks for your efforts and contribution to the community!.

hhy3 · 2023-12-19T03:07:43Z

/kind feature

alexanderguzhva · 2023-12-21T15:21:50Z

include/knowhere/comp/index_param.h

@@ -43,6 +43,7 @@ constexpr const char* INDEX_RAFT_IVFPQ = "GPU_RAFT_IVF_PQ";
 constexpr const char* INDEX_RAFT_CAGRA = "GPU_RAFT_CAGRA";

 constexpr const char* INDEX_HNSW = "HNSW";
+constexpr const char* INDEX_HNSW_SQ = "HNSW_SQ";


not HNSW_SQ8?

it should be HNSW_SQ8

alexanderguzhva · 2023-12-21T15:24:47Z

include/knowhere/utils.h

+hash_u8_vec(const uint8_t* x, size_t d) {
+    uint64_t h = seed;
+    for (size_t i = 0; i < d; ++i) {
+        h = h * 13331 + *(uint8_t*)(x + i);


h = h * 13331 + x[i];

alexanderguzhva · 2023-12-21T15:30:22Z

src/simd/distances_avx.cc

@@ -215,5 +215,28 @@ fvec_L2sqr_batch_4_avx(const float* x, const float* y0, const float* y1, const f
 }
 FAISS_PRAGMA_IMPRECISE_FUNCTION_END

+// trust the compiler to unroll this properly
+int32_t


please confirm that it is int32_t, not uint32_t

There seems no big difference between int32_t and uint32_t

alexanderguzhva · 2023-12-21T15:40:25Z

thirdparty/hnswlib/hnswlib/hnswalg.h

+            data_norm = knowhere::CopyAndNormalizeVecs(from, 1, dim);
+            from = data_norm.get();
+        }
+        for (size_t i = 0; i < dim; ++i) {


please confirm that is it really int8_t, not uint8_t.
Classical SQ implementation uses uint8_t and maps into [0, 255] range, not [-128, 127].

SQ used there is symmetric version which maps to [-127, 127], which unify ip and l2 metric and simplify implementation.

It will be good to add some rough comments about SQ types here.

chasingegg · 2023-12-21T02:52:32Z

src/index/hnsw/hnsw_config.h

@@ -85,6 +85,14 @@ class HnswConfig : public BaseConfig {
    }
 };

+class HnswSQConfig : public HnswConfig {


can we use the same user api with scann, by using "with_raw_data" option? Also, do we need another refine param like "reorder_k" used in scann

alexanderguzhva · 2023-12-26T15:44:49Z

thirdparty/hnswlib/hnswlib/hnswalg.h

    mutable knowhere::lru_cache<uint64_t, tableint> lru_cache;

+    void
+    trainSQuant(const float* train_data, size_t ntrain) {
+        size_t dim = *(size_t*)dist_func_param_;


Please add alpha_ = 0; here

alexanderguzhva · 2023-12-26T15:46:30Z

thirdparty/hnswlib/hnswlib/hnswalg.h

+        }
+        for (size_t i = 0; i < dim; ++i) {
+            float x = from[i] / alpha_;
+            if (x > 1.0f) {


So, this implementation assumes that the data distribution is symmetric around 0, correct?

Yes. It's the most common scenario

alexanderguzhva · 2023-12-26T15:47:28Z

thirdparty/hnswlib/hnswlib/hnswalg.h

+            _mm_prefetch(getSQDataByInternalId(id), _MM_HINT_T0);
+        } else {
+            _mm_prefetch(getDataByInternalId(id), _MM_HINT_T0);
+        }


just mentioning that this is x86-only code, unless USE_PREFETCH is enabled for x86 only as well

We need to centralized these platform related code just like what we do for distance hook. Created an issue to track #416.

using the hook mechanism for prefetching is a horrible idea

alexanderguzhva · 2023-12-26T15:52:35Z

thirdparty/hnswlib/hnswlib/space_cosine.h

 class CosineSpace : public SpaceInterface<float> {
    DISTFUNC<float> fstdistfunc_;
+    DISTFUNC<float> fstdistfunc_sq_;


The introduction of an additional entity fstdistfunc_sq_ is needed, because SpaceInterface<> struct does not distinguish between underlying datatypes, right?

liliu-z · 2024-02-27T04:27:19Z

src/index/hnsw/hnsw.cc

@@ -27,6 +27,7 @@
 #include "knowhere/utils.h"

 namespace knowhere {
+template <bool USE_SQ>


specify SQ8 here

liliu-z · 2024-02-27T05:47:11Z

src/index/hnsw/hnsw.cc

@@ -477,7 +489,7 @@ class HnswIndexNode : public IndexNode {

    std::string
    Type() const override {
-        return knowhere::IndexEnum::INDEX_HNSW;
+        return USE_SQ ? knowhere::IndexEnum::INDEX_HNSW_SQ8 : knowhere::IndexEnum::INDEX_HNSW;


use constexpr

liliu-z · 2024-02-27T05:48:23Z

src/index/hnsw/hnsw.cc

@@ -27,6 +27,7 @@
 #include "knowhere/utils.h"

 namespace knowhere {
+template <bool USE_SQ>


Let's create a enum class type for this, in case we have SQ8, SQ4, PQ in the future

liliu-z · 2024-02-27T06:12:46Z

thirdparty/hnswlib/hnswlib/hnswalg.h

@@ -67,11 +67,13 @@ class HierarchicalNSW : public AlgorithmInterface<dist_t> {
    }

    HierarchicalNSW(SpaceInterface<dist_t>* s, size_t max_elements, size_t M = 16, size_t ef_construction = 200,
-                    size_t random_seed = 100)
+                    bool use_sq = false, bool use_sq_refine = false, size_t random_seed = 100)


these two new params are coupled. Let's make a new constructor for SQ

Or why not make SQ a template the same as HNSWIndexNode

liliu-z · 2024-02-27T06:21:59Z

thirdparty/hnswlib/hnswlib/hnswalg.h

        offsetData_ = size_links_level0_;
+        if (use_sq) {


Mixed usage of use_sq and sq_enabled_

Ok I'll fix it

liliu-z · 2024-02-27T06:33:06Z

thirdparty/hnswlib/hnswlib/hnswalg.h

    mutable knowhere::lru_cache<uint64_t, tableint> lru_cache;

+    void
+    trainSQuant(const float* train_data, size_t ntrain) {


We are adopting symmetrical global SQ. Plz make a note here.

liliu-z · 2024-02-27T06:34:23Z

thirdparty/hnswlib/hnswlib/hnswalg.h

+            data_norm = knowhere::CopyAndNormalizeVecs(from, 1, dim);
+            from = data_norm.get();
+        }
+        for (size_t i = 0; i < dim; ++i) {


It will be good to add some rough comments about SQ types here.

liliu-z · 2024-02-27T07:08:56Z

thirdparty/hnswlib/hnswlib/hnswalg.h

+            _mm_prefetch(getSQDataByInternalId(id), _MM_HINT_T0);
+        } else {
+            _mm_prefetch(getDataByInternalId(id), _MM_HINT_T0);
+        }


We need to centralized these platform related code just like what we do for distance hook. Created an issue to track #416.

Signed-off-by: zh Wang <[email protected]>

liliu-z · 2024-03-08T11:01:55Z

/lgtm

sre-ci-robot · 2024-03-08T11:02:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hhy3, liliu-z

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hhy3,liliu-z]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hhy3 force-pushed the support_sq branch from 78e0e55 to 218e029 Compare December 19, 2023 03:00

mergify bot added dco-passed do-not-merge/missing-related-issue labels Dec 19, 2023

sre-ci-robot requested review from cqy123456 and liliu-z December 19, 2023 03:01

sre-ci-robot added approved size/L labels Dec 19, 2023

sre-ci-robot added the kind/feature label Dec 19, 2023

mergify bot removed the do-not-merge/missing-related-issue label Dec 19, 2023

hhy3 force-pushed the support_sq branch from 218e029 to 82a4f81 Compare December 19, 2023 06:03

sre-ci-robot added size/XL and removed size/L labels Dec 19, 2023

hhy3 force-pushed the support_sq branch 2 times, most recently from 15978fe to 8740a81 Compare December 20, 2023 08:50

mergify bot added the ci-passed label Dec 20, 2023

alexanderguzhva reviewed Dec 21, 2023

View reviewed changes

chasingegg reviewed Dec 22, 2023

View reviewed changes

hhy3 force-pushed the support_sq branch from 8740a81 to cf1e4d4 Compare December 22, 2023 09:17

mergify bot removed the ci-passed label Dec 22, 2023

hhy3 force-pushed the support_sq branch 2 times, most recently from 9340554 to fd8a399 Compare December 25, 2023 13:33

mergify bot added the ci-passed label Dec 25, 2023

alexanderguzhva reviewed Dec 26, 2023

View reviewed changes

liliu-z reviewed Feb 27, 2024

View reviewed changes

hhy3 force-pushed the support_sq branch from fd8a399 to dc77776 Compare March 4, 2024 13:07

mergify bot removed the ci-passed label Mar 4, 2024

Add sq for hnsw

c6d955b

Signed-off-by: zh Wang <[email protected]>

hhy3 force-pushed the support_sq branch from dc77776 to c6d955b Compare March 5, 2024 09:56

mergify bot added the ci-passed label Mar 6, 2024

sre-ci-robot assigned liliu-z Mar 8, 2024

sre-ci-robot added the lgtm label Mar 8, 2024

liliu-z approved these changes Mar 8, 2024

View reviewed changes

sre-ci-robot merged commit e44ba8d into zilliztech:main Mar 8, 2024
9 checks passed

Add sq for hnsw #311

Add sq for hnsw #311

Conversation

hhy3 commented Dec 19, 2023 • edited Loading

mergify bot commented Dec 19, 2023

hhy3 commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liliu-z commented Mar 8, 2024

sre-ci-robot commented Mar 8, 2024

hhy3 commented Dec 19, 2023 •

edited

Loading