Changing serialization for knn vector from single array object to collection of floats by martin-gaievski · Pull Request #253 · opensearch-project/k-NN

martin-gaievski · 2022-01-08T03:16:57Z

Description

Changing logic of k-NN vector serialization/deserialization in order to decrease memory taken by the index. Existing serialization stores vector as Java array, which has 27 bytes of overhead for each individual array (as per stream protocol). In newly implemented logic vector is serialized as a collection of individual numbers. During deserialization we can restore this collection to an array assuming it length from number of bytes in byte stream.

Logic has been added in a backward compatibility manner for deserialization. Initially we read first 27 bytes of the byte stream and identify if this is a serialized array based on the same stream protocol grammar. Depending on result of the check we apply old logic (deserialize as a single array object) or a new logic (deserialize as collection of individual floats).

Performance stayed roughly the same time-wise but we see an improvement in memory usage. Below are results of 2 tests comparing existing and new approaches. For testing we have used the benchmark tool from main k-NN branch with two data sets Fashion-MNIST and SIFT from here.

Diff (changed - original) for SIFT dataset:

{
  "test_took": -37602.98385410011,
  "train_model_took_total": -717.2553265995593,
  "create_index_took_total": 58.110593299596985,
  "ingest_took_total": 248.3000000000029,
  "ingest_took_p50": -0.5999999999999943,
  "ingest_took_p90": -1.799999999999983,
  "ingest_took_p99": -0.6000000000000227,
  "refresh_index_store_kb_total": -14444.896679687488,
  "refresh_index_took_total": -152.2593566997159,
  "force_merge_took_total": -119.27976410053702,
  "query_took_total": -36920.59999999998,
  "query_took_p50": -0.6999999999999993,
  "query_took_p90": -1.0999999999999979,
  "query_took_p99": -2.6999999999999993,
  "query_memory_kb_total": -0.8000000000029104,
  "query_recall@K_total": 0.004822199999999999,
  "query_recall@1_total": -0.00017999999999984695
}

Diff (changed - original) for Fashion-MNIST dataset:

{
  "test_took": -1796.963270999724,
  "create_index_took_total": -195.58473960000993,
  "ingest_took_total": 158.40000000000146,
  "ingest_took_p50": -0.19999999999998863,
  "ingest_took_p90": -0.19999999999998863,
  "ingest_took_p99": 4.0,
  "refresh_index_store_kb_total": 23103.330078125,
  "refresh_index_took_total": -4277.6522125998745,
  "force_merge_took_total": 60.2736812000615,
  "query_took_total": 2457.600000000035,
  "query_took_p50": -0.20000000000000018,
  "query_took_p90": -0.1999999999999993,
  "query_took_p99": -0.6999999999999993,
  "query_memory_kb_total": -1.5,
  "query_recall@K_total": 8.959999999724744e-06,
  "query_recall@1_total": 0.0
}

Issues Resolved

knn_vector codec minor overhead

Check List

New functionality includes testing.
- All tests pass
Commits are signed as per the DCO using --signoff

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.

martin-gaievski · 2022-01-08T03:17:35Z

Adding results of performance tests perf_test_results.zip

codecov-commenter · 2022-01-08T03:32:26Z

Codecov Report

Merging #253 (5508fa8) into main (fee4026) will increase coverage by 0.09%.
The diff coverage is 86.56%.

@@             Coverage Diff              @@
##               main     #253      +/-   ##
============================================
+ Coverage     83.22%   83.32%   +0.09%     
- Complexity      865      883      +18     
============================================
  Files           123      127       +4     
  Lines          3780     3832      +52     
  Branches        359      361       +2     
============================================
+ Hits           3146     3193      +47     
- Misses          473      477       +4     
- Partials        161      162       +1

Impacted Files	Coverage Δ
...n/java/org/opensearch/knn/index/KNNIndexShard.java	`92.85% <ø> (ø)`
...nn/index/codec/KNN80Codec/KNN80CompoundFormat.java	`100.00% <ø> (ø)`
...index/codec/KNN80Codec/KNN80DocValuesConsumer.java	`82.00% <ø> (ø)`
...n/index/codec/util/KNNVectorAsArraySerializer.java	`62.50% <62.50%> (ø)`
.../util/KNNVectorAsCollectionOfFloatsSerializer.java	`85.71% <85.71%> (ø)`
...n/index/codec/util/KNNVectorSerializerFactory.java	`96.15% <96.15%> (ø)`
...opensearch/knn/index/KNNVectorScriptDocValues.java	`90.47% <100.00%> (+8.65%)`	⬆️
...ain/java/org/opensearch/knn/index/VectorField.java	`75.00% <100.00%> (-6.82%)`	⬇️
.../opensearch/knn/index/codec/util/KNNCodecUtil.java	`93.75% <100.00%> (ø)`
...search/knn/index/codec/util/SerializationMode.java	`100.00% <100.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fee4026...5508fa8. Read the comment docs.

martin-gaievski · 2022-01-10T16:48:37Z

+import java.util.Random;
+import java.util.stream.IntStream;
+
+public class VectorSerializerTests extends KNNTestCase {


Adding unit tests to keep percentage of code coverage greater or equals to the current one

Shivamdhar

looks good!

VijayanB · 2022-01-12T19:50:01Z

+     * @param byteStream stream of bytes that will be used for deserialization to array of floats
+     * @return array of floats deserialized from the stream
+     * @throws IOException
+     * @throws ClassNotFoundException


Do we need to have ClassNotFoundException here?

It comes from the serializer implementation, ObjectInputStream.readObject().
I've dig deeper into exceptions and seems we just catch and re-throw RuntimeException at the higher level, so I moved this logic inside the serializer and removed all exceptions from the interface method signature. I hope it makes sense

VijayanB · 2022-01-12T19:51:40Z

+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ *
+ * Modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.


we don't need to expand license description

Sure, let me revert to a shorter header, seems this one has been added by signoff command automatically

VijayanB · 2022-01-12T19:52:13Z

+/*
+ * 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.
+ *
+ * Modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */


Suggested change

/*

* 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.

*

* Modifications Copyright OpenSearch Contributors. See

* GitHub history for details.

*/

/*

* Copyright OpenSearch Contributors

* SPDX-License-Identifier: Apache-2.0

*/

VijayanB · 2022-01-12T19:54:55Z

+    public float[] byteToFloatArray(ByteArrayInputStream byteStream) {
+        final byte[] vectorAsByteArray = new byte[byteStream.available()];
+        byteStream.read(vectorAsByteArray, 0, byteStream.available());
+        final float[] vector = new float[vectorAsByteArray.length / BYTES_IN_FLOAT];


shall we move it to a variable

agree, it will make code more readable. let me do the change

VijayanB · 2022-01-12T19:55:57Z

+    }
+
+    private static byte highByte(short shortValue) {
+        return (byte) (shortValue>>8);


need constant instead of number

agree, changing in next revision

VijayanB · 2022-01-12T19:57:37Z

+
+    public void testVectorSerializerFactory() {
+        final KNNVectorSerializer defaultSerializer = KNNVectorSerializerFactory.getDefaultSerializer();
+        assertNotNull(defaultSerializer);


shall we assert what is the default Serializer type you are expecting too.

I can check on exact serializer based on functionality it supports. Current default is for collection of floats, so it should be able to deserialize collection of floats without any exception and to the same original content. Is this something you're looking for in such check?

…lection of floats Signed-off-by: Martin Gaievski <gaievski@amazon.com>

-added getDefaultSerializer to Factory - moved SerializationMode enum to a separate file - added javadocs and comments - adjust format, added missing endline characters Signed-off-by: Martin Gaievski <gaievski@amazon.com>

- replace Vector by KNNVector in class names and variables - fixed method names in Serializer interface - replace number of bytes in float from number to constant Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

- rework factory method getSerializerByStreamContent - added test case for stream of unsupported content - removed exceptions from Serializer interface method's signatures, changed it to unchecked runtime exception - simplify license header in new classes Signed-off-by: Martin Gaievski <gaievski@amazon.com>

jmazanec15

LGTM thanks!

…lection of floats (opensearch-project#253) * Changing serialization for knn vector from single array object to collection of floats rev2: * Addressing PR comments: - added getDefaultSerializer to Factory - moved SerializationMode enum to a separate file - added javadocs and comments - adjust format, added missing endline characters rev3: * Addressing multiple review comments: - replace Vector by KNNVector in class names and variables - fixed method names in Serializer interface - replace number of bytes in float from number to constant rev4: * Moving new classes under index.codec.util rev5: * Addressing multiple review comments: - rework factory method getSerializerByStreamContent - added test case for stream of unsupported content - removed exceptions from Serializer interface method's signatures, changed it to unchecked runtime exception - simplify license header in new classes Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Previously, we had serialized vectors via an array method. This was inefficient and remove in the opensearch-project#253 PR and launced in 1.3.0. With that, we no longer serialized new segment data via the array based serializer. Now that it is 3.0, because the oldest index that can be upgraded is from 1.3.0, we no longer need to handle array based serialization. So, we can remove all of it. Signed-off-by: John Mazanec <jmazane@amazon.com>

Previously, we had serialized vectors via an array method. This was inefficient and remove in the #253 PR and launced in 1.3.0. With that, we no longer serialized new segment data via the array based serializer. Now that it is 3.0, because the oldest index that can be upgraded is from 1.3.0, we no longer need to handle array based serialization. So, we can remove all of it. Signed-off-by: John Mazanec <jmazane@amazon.com>

Previously, we had serialized vectors via an array method. This was inefficient and remove in the opensearch-project#253 PR and launced in 1.3.0. With that, we no longer serialized new segment data via the array based serializer. Now that it is 3.0, because the oldest index that can be upgraded is from 1.3.0, we no longer need to handle array based serialization. So, we can remove all of it. Signed-off-by: John Mazanec <jmazane@amazon.com>

…lection of floats (opensearch-project#253) * Changing serialization for knn vector from single array object to collection of floats rev2: * Addressing PR comments: - added getDefaultSerializer to Factory - moved SerializationMode enum to a separate file - added javadocs and comments - adjust format, added missing endline characters rev3: * Addressing multiple review comments: - replace Vector by KNNVector in class names and variables - fixed method names in Serializer interface - replace number of bytes in float from number to constant rev4: * Moving new classes under index.codec.util rev5: * Addressing multiple review comments: - rework factory method getSerializerByStreamContent - added test case for stream of unsupported content - removed exceptions from Serializer interface method's signatures, changed it to unchecked runtime exception - simplify license header in new classes Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Previously, we had serialized vectors via an array method. This was inefficient and remove in the opensearch-project#253 PR and launced in 1.3.0. With that, we no longer serialized new segment data via the array based serializer. Now that it is 3.0, because the oldest index that can be upgraded is from 1.3.0, we no longer need to handle array based serialization. So, we can remove all of it. Signed-off-by: John Mazanec <jmazane@amazon.com>

martin-gaievski commented Jan 10, 2022

View reviewed changes

martin-gaievski self-assigned this Jan 10, 2022

martin-gaievski added v1.3.0 Issues and PRs related to version 1.3.0 Enhancements Increases software capabilities beyond original client specifications labels Jan 10, 2022

martin-gaievski linked an issue Jan 10, 2022 that may be closed by this pull request

knn_vector codec minor overhead #53

Closed

martin-gaievski requested review from VijayanB, jmazanec15, naveentatikonda and vamshin January 10, 2022 17:01

jmazanec15 reviewed Jan 10, 2022

View reviewed changes

Shivamdhar reviewed Jan 11, 2022

View reviewed changes

Comment thread src/main/java/org/opensearch/knn/index/KNNVectorScriptDocValues.java Outdated

Shivamdhar reviewed Jan 11, 2022

View reviewed changes

Comment thread src/main/java/org/opensearch/knn/index/codec/VectorAsArraySerializer.java Outdated

Shivamdhar reviewed Jan 11, 2022

View reviewed changes

Comment thread src/main/java/org/opensearch/knn/index/codec/VectorSerializerFactory.java Outdated

VijayanB reviewed Jan 11, 2022

View reviewed changes

Comment thread src/main/java/org/opensearch/knn/index/VectorField.java Outdated

VijayanB reviewed Jan 11, 2022

View reviewed changes

Comment thread src/main/java/org/opensearch/knn/index/codec/VectorAsArraySerializer.java Outdated

VijayanB reviewed Jan 11, 2022

View reviewed changes

Comment thread src/main/java/org/opensearch/knn/index/codec/VectorAsCollectionOfFloatsSerializer.java Outdated

VijayanB reviewed Jan 11, 2022

View reviewed changes

Comment thread src/main/java/org/opensearch/knn/index/codec/VectorAsCollectionOfFloatsSerializer.java Outdated

martin-gaievski force-pushed the custom_serialization_for_vector branch from e7d2593 to da398af Compare January 11, 2022 23:10

martin-gaievski marked this pull request as ready for review January 12, 2022 18:43

martin-gaievski requested review from a team, Shivamdhar, VijayanB and jmazanec15 January 12, 2022 18:43

Shivamdhar previously approved these changes Jan 12, 2022

View reviewed changes

VijayanB reviewed Jan 12, 2022

View reviewed changes

VijayanB previously approved these changes Jan 12, 2022

View reviewed changes

martin-gaievski dismissed stale reviews from VijayanB and Shivamdhar via 5508fa8 January 12, 2022 23:46

martin-gaievski force-pushed the custom_serialization_for_vector branch 2 times, most recently from 186bf04 to 8e946f5 Compare January 13, 2022 00:12

martin-gaievski added 5 commits January 12, 2022 16:26

Changing serialization for knn vector from single array object to col…

ac4519c

…lection of floats Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Addressing PR comments:

4fb45b3

-added getDefaultSerializer to Factory - moved SerializationMode enum to a separate file - added javadocs and comments - adjust format, added missing endline characters Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Addressing multiple review comments:

953cd1b

- replace Vector by KNNVector in class names and variables - fixed method names in Serializer interface - replace number of bytes in float from number to constant Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Moving new classes under index.codec.util

38cb2f9

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski force-pushed the custom_serialization_for_vector branch from 8e946f5 to 7a3a1a9 Compare January 13, 2022 00:27

martin-gaievski requested a review from Shivamdhar January 13, 2022 16:54

jmazanec15 approved these changes Jan 13, 2022

View reviewed changes

martin-gaievski requested a review from VijayanB January 13, 2022 17:26

VijayanB approved these changes Jan 13, 2022

View reviewed changes

martin-gaievski merged commit e781ad3 into opensearch-project:main Jan 13, 2022

martin-gaievski mentioned this pull request Sep 25, 2023

Add graph creation stats to the KNNStats API #1141

Merged

5 tasks

jmazanec15 mentioned this pull request Mar 6, 2025

Removes Array based vector serialization #2587

Merged

1 task

Conversation

martin-gaievski commented Jan 8, 2022

Description

Issues Resolved

Check List

Uh oh!

martin-gaievski commented Jan 8, 2022

Uh oh!

codecov-commenter commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Shivamdhar left a comment

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!

Choose a reason for hiding this comment

Uh oh!

VijayanB Jan 12, 2022 • 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!

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!

jmazanec15 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jan 8, 2022 •

edited

Loading

VijayanB Jan 12, 2022 •

edited

Loading