add key-value file! #2

ruclz · 2022-10-24T07:29:30Z

Oceanbase sort needs key-value struct. But the avx512-64bits-qsort only supports key sort. We need key-value sort in this lib.

xtangxtang · 2022-11-17T07:24:01Z

The PR is to meet the requirement from Oceanbase.

Oceanbase is a Key/Value store Database. It uses std::sort to sort the object during creating index or other jobs. Not like just sort the key, Oceanbase passes inner ObStore class object as an array to do the sort job. And the std::sort callback comparing function to determine how to get the key from the ObStore to compare. So in Oceanbase, it swaps the ObStore object index in the array according to the sort function.

Since the avx512 sort in this project do a wondful work in bitonic sort, we want to reuse the core code. But the sort API just do the sort work, doesn't return the indexes of the key after sorted, it is hard for us to integrated the code into Oceanbase. So we create the PR to modify the sort API from void avx512_qsort(uint16_t *arr, int64_t arrsize) to void avx512_qsort(uint16_t *arr,uint64_t *indexes, int64_t arrsize'). The returned indexes is used to record the sorted key indexes and help API caller to swap the its objec array outside.

Currently we have created a PR in Oceanbase to merge this project code as a 3rd party lib, now is wating for this PR approval.

We did Create index performance test for Oceanbase 3.1 version:
creating index can speed up from 20% ~ 47% according to different index type. The performance workload is SSB. The table and creating index steps is as following SQL:

create table lineitem (
l_orderkey bigint not null,
l_partkey bigint not null,
l_suppkey bigint not null,
l_linenumber bigint not null,
l_quantity bigint not null,
l_extendedprice bigint not null,
l_discount bigint not null,
l_tax bigint not null,
l_returnflag char(1) default null,
l_linestatus char(1) default null,
l_shipdate date not null,
l_commitdate date default null,
l_receiptdate date default null,
l_shipinstruct char(25) default null,
l_shipmode char(10) default null,
l_comment varchar(44) default null,
primary key(l_orderkey, l_linenumber))
tablegroup = tpch_tg_100g_lineitem_order_group
partition by key (l_orderkey) partitions 192;

create index I_L_ORDERKEY on lineitem(l_orderkey) local;
create index I_L_SHIPINS on lineitem(l_shipinstruct) local;
create index I_L_SHIPDATE_COMMITDATE on lineitem(l_shipdate, l_commitdate) local;

drop index I_L_ORDERKEY on lineitem;
drop index I_L_SHIPINS on lineitem;
drop index I_L_SHIPDATE_COMMITDATE on lineitem;

The performance is test on Intel(R) Xeon(R) Gold 6252 CPU @ 2.10GHz and the results is (unit is second):

The performance result is as following:

r-devulap · 2023-01-19T21:21:26Z

.vscode/settings.json

@@ -0,0 +1,57 @@
+{


Please remove this file.

Makefile

r-devulap

As far as I understood, you are essentially adding argsort. The modified indices would sort the original array. Correct? Since this is a new feature, could you add new unit tests that cover this feature?

In general, the PR needs a lot of formatting changes. The diff is pretty hard to read. Could you fix the formatting and run clang-format on the files and re-submit? The formatting specifications are in a _clang-format file.

r-devulap · 2023-01-19T23:42:46Z

On a second thought, argsort seems like a special case. You are sorting both key and value arrays.

ruclz · 2023-01-29T02:30:06Z

As far as I understood, you are essentially adding argsort. The modified indices would sort the original array. Correct? Since this is a new feature, could you add new unit tests that cover this feature?

In general, the PR needs a lot of formatting changes. The diff is pretty hard to read. Could you fix the formatting and run clang-format on the files and re-submit? The formatting specifications are in a _clang-format file.

Hello, I run the clang_format execution in the project, but I got the error like the figure, can you have some suggests to solve it? Also, the unit tests can't run on my environment including the unit tests that you have completed, can you share your environment to me, like OS, compiler version and so on.

ruclz · 2023-01-29T02:32:20Z

On a second thought, argsort seems like a special case. You are sorting both key and value arrays.

OK, we are sorting key, and the value column exchange with each other according to key.

r-devulap · 2023-01-31T20:54:00Z

Hello, I run the clang_format execution in the project, but I got the error like the figure, can you have some suggests to solve it?

Hmm, not sure why you see that error. I am able to run clang-format on your patch just fine.

Also, the unit tests can't run on my environment including the unit tests that you have completed, can you share your environment to me, like OS, compiler version and so on.

Do you see an error? I am using ubuntu latest, g++-10 and running on a SKX. If your processor doesnt support AVX-512, you could run on an SDE too with this command: sde-external-9.14.0-2022-10-25-lin/sde64 -icx -- ./testexe

ruclz · 2023-02-13T06:55:41Z

OK, I added a unittest for key-value sort, and I ran clang_format on all files I modified. Please help to review. Thanks.

r-devulap · 2023-02-21T22:43:55Z

Hi @ruclz This PR modifies the existing avx512_qsort<T>() function signature by adding another argument to the function and also adds unnecessary code if someone wants to use only data sorting and not key-value sort. I think a better option would be to move your code to new header files avx512_64bit_keyvaluesort.hpp and avx512-common-keyvaluesort.h . I understand there might be some code duplication but we can work on minimizing that in a separate PR.

r-devulap

Move code to separate files and add new API's avx512_keyvaluesort<T>().

ruclz · 2023-02-23T06:03:33Z

Hi, @r-devulap, I have moved my code to new header files avx512-64bit-keyvaluesort.hpp and avx512-common-keyvaluesort.h. The unit test can run correctly, please help to review, thanks.

ruclz · 2023-02-23T06:18:57Z

Sorry, I forgot to merge the test_all.cpp in the last commit.

r-devulap

Still working on reviewing the code, meanwhile lets clean up all the duplicated code.

src/avx512-64bit-keyvaluesort.hpp

r-devulap · 2023-02-23T22:28:32Z

src/avx512-64bit-keyvaluesort.hpp

+        qsort_64bit_<vtype>(keys, indexes, pivot_index, right, max_iters - 1);
+}
+
+X86_SIMD_SORT_INLINE int64_t replace_nan_with_inf_kv(double *arr,


This is the same function as in avx512-64bit-qsort.hpp. You could move this to the common header file as well and no need to re-write it.

src/avx512-64bit-keyvaluesort.hpp

r-devulap · 2023-02-23T22:30:37Z

src/avx512-common-keyvaluesort.h

+#else
+#define X86_SIMD_SORT_INLINE static
+#define X86_SIMD_SORT_FINLINE static
+#endif


lines 36-82 is duplicated. Move them to new header file and include that here and avx512-common-qsort.h

r-devulap · 2023-02-23T22:31:41Z

src/avx512-16bit-qsort.hpp

@@ -695,7 +695,7 @@ void avx512_qsort(int16_t *arr, int64_t arrsize)
 }

 template <>
-void avx512_qsort(uint16_t *arr, int64_t arrsize)
+inline void avx512_qsort(uint16_t *arr, int64_t arrsize)


same. get rid of inline please.

src/avx512-32bit-qsort.hpp

r-devulap · 2023-02-23T22:31:57Z

src/avx512-32bit-qsort.hpp

@@ -691,7 +691,7 @@ void avx512_qsort<int32_t>(int32_t *arr, int64_t arrsize)
 }

 template <>
-void avx512_qsort<uint32_t>(uint32_t *arr, int64_t arrsize)
+inline void avx512_qsort<uint32_t>(uint32_t *arr, int64_t arrsize)


src/avx512-32bit-qsort.hpp

r-devulap · 2023-02-23T22:33:36Z

src/avx512-64bit-keyvaluesort.hpp

+
+template <>
+inline void
+avx512_qsort_kv<int64_t>(int64_t *keys, uint64_t *indexes, int64_t arrsize)


Are indices always expected to be a uint64_t ?

r-devulap · 2023-02-27T18:05:05Z

@ruclz Could you add benchmarking to know how fast this is relative to using std::sort? From issue #8:

avx512_qsort_kv(double *keys, uint64_t *indexes, int64_t arrsize) is a little worse than my simple bubble sorting.

ruclz · 2023-02-28T03:38:53Z

@ruclz Could you add benchmarking to know how fast this is relative to using std::sort? From issue #8:

avx512_qsort_kv(double *keys, uint64_t *indexes, int64_t arrsize) is a little worse than my simple bubble sorting.

OK, I am coding for benchmarking.

ruclz · 2023-03-01T08:24:38Z

@ruclz Could you add benchmarking to know how fast this is relative to using std::sort? From issue #8:

avx512_qsort_kv(double *keys, uint64_t *indexes, int64_t arrsize) is a little worse than my simple bubble sorting.

The figure shows the performance of key-value bitonic sort and std::sort. There is no performance drop compared with std::sort.

r-devulap · 2023-03-03T18:44:44Z

benchmarks/bench.hpp

+
+    for (uint64_t ii = 0; ii < iters; ++ii) {
+        start = cycles_start();
+        std::sort(sortedaar_bckup.begin(),


Not sure if std::sort is a fair comparison. The way the key-values are arranged in memory for std::vector<sorted_t<K, V>> is very different when compared two distinct vectors std::vector<K>, std::vector<V>. I would think this can alter performance significantly because you can no longer load continuous chunk of memory into ZMM registers. Does Oceanbase have two separate vectors for key and values (I assume they do cos that's the way you implemented the sort). How does Oceanbase currently sort them?

ObStoreRow is the base unit to do the sort in Oceanbase. The key and value are all ObObject class type in the ObStoreRow. There is no sperate vector for key and value. The ObStoreRow is passed to the std::sort in Oceanbase. std::sort first get the key type from ObObject and then do the sort work.

To enable the AVX512 sort, we need extract the key from the ObStoreRow and then pass the key and ObStoreRow as the value to avx512_sort(key, value) like API. That's way we need the key-value interface

benchmarks/bench-tgl.out

src/avx512-64bit-keyvaluesort.hpp

…12-64bit-common.h, modify the bench-tgl.out to origin file.

ruclz · 2023-03-09T08:01:11Z

We have moved the get_povit_64bit function and sort_zmm_64bit function to avx512-64bit-common.h, and modified the bench-tgl.out to origin file. Please help to review again, thanks.

r-devulap · 2023-03-09T23:15:51Z

@ruclz please fix the author name and get rid of the unnecessary struct and we should be good to go.

ruclz · 2023-03-10T01:42:51Z

Yeah, we have fixed the authors name in the last commit.

r-devulap · 2023-03-13T20:23:26Z

LGTM, Thanks @ruclz!

r-devulap · 2023-03-13T21:15:13Z

Did a little bit of code cleanup, see #15

add key-value file!

8335a0b

ruclz marked this pull request as draft October 24, 2022 07:32

Completed the key-value sort with uint64_t.

156142a

r-devulap reviewed Jan 19, 2023

View reviewed changes

.vscode/settings.json Outdated

@@ -0,0 +1,57 @@

{

Copy link

Contributor

r-devulap Jan 19, 2023

Choose a reason for hiding this comment

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

Please remove this file.

r-devulap reviewed Jan 19, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

r-devulap reviewed Jan 19, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

r-devulap requested changes Jan 19, 2023

View reviewed changes

ruclz and others added 9 commits February 13, 2023 13:54

Merge branch 'main' into main

473427c

first

a05f062

first

fad1753

run clang-format and add unit test for key-value sort

7821e01

add unit test and run clang_format

ae02b58

Rename Makefile to Makefile.bak2

5363ba4

Rename Makefile.bak to Makefile

cd836c7

Delete Makefile.bak2

541f914

Update meson.build

8873ea1

r-devulap mentioned this pull request Feb 17, 2023

Argsort Adaptation #7

Closed

r-devulap marked this pull request as ready for review February 21, 2023 22:44

r-devulap requested changes Feb 21, 2023

View reviewed changes

ruclz added 3 commits February 23, 2023 13:54

Creat key-value sort hpp file.

7ea8230

Creat key-value sort hpp file.

09ef43a

Creat key-value sort hpp file.

17b534c

Sorry, I forgot to merge the test_all.cpp in the last commit.

4927fe2

r-devulap requested changes Feb 23, 2023

View reviewed changes

r-devulap mentioned this pull request Feb 24, 2023

Can I use this lib to output the original index of these sorted data #8

Open

Delete duplicated code and add benchmarking code of key value sort

e73aa3f

r-devulap reviewed Mar 3, 2023

View reviewed changes

benchmarks/bench-tgl.out Outdated Show resolved Hide resolved

r-devulap requested changes Mar 8, 2023

View reviewed changes

src/avx512-64bit-keyvaluesort.hpp Outdated Show resolved Hide resolved

Move the get_povit_64bit function and sort_zmm_64bit function to avx5…

b73ba45

…12-64bit-common.h, modify the bench-tgl.out to origin file.

Fix the authors name.

c083424

Change the author of avx512-64bit-common.h file to Rahu.

ecc94cf

r-devulap merged commit 28c0b3e into intel:main Mar 13, 2023

Bulat-Ziganshin mentioned this pull request Jun 2, 2023

Consider checking out vqsort performance ClickHouse/ClickHouse#46348

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add key-value file! #2

add key-value file! #2

ruclz commented Oct 24, 2022

xtangxtang commented Nov 17, 2022 •

edited

Loading

r-devulap Jan 19, 2023

r-devulap left a comment •

edited

Loading

r-devulap commented Jan 19, 2023

ruclz commented Jan 29, 2023

ruclz commented Jan 29, 2023 •

edited

Loading

r-devulap commented Jan 31, 2023

ruclz commented Feb 13, 2023 •

edited

Loading

r-devulap commented Feb 21, 2023

r-devulap left a comment •

edited

Loading

ruclz commented Feb 23, 2023

ruclz commented Feb 23, 2023

r-devulap left a comment

r-devulap Feb 23, 2023

r-devulap Feb 23, 2023

r-devulap Feb 23, 2023

r-devulap Feb 23, 2023

r-devulap Feb 23, 2023

r-devulap commented Feb 27, 2023

ruclz commented Feb 28, 2023

ruclz commented Mar 1, 2023

r-devulap Mar 3, 2023

xtangxtang Mar 9, 2023

xtangxtang Mar 9, 2023

ruclz commented Mar 9, 2023

r-devulap commented Mar 9, 2023

ruclz commented Mar 10, 2023

r-devulap commented Mar 13, 2023

r-devulap commented Mar 13, 2023

add key-value file! #2

add key-value file! #2

Conversation

ruclz commented Oct 24, 2022

xtangxtang commented Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

r-devulap left a comment • edited Loading

Choose a reason for hiding this comment

r-devulap commented Jan 19, 2023

ruclz commented Jan 29, 2023

ruclz commented Jan 29, 2023 • edited Loading

r-devulap commented Jan 31, 2023

ruclz commented Feb 13, 2023 • edited Loading

r-devulap commented Feb 21, 2023

r-devulap left a comment • edited Loading

Choose a reason for hiding this comment

ruclz commented Feb 23, 2023

ruclz commented Feb 23, 2023

r-devulap left a 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

r-devulap commented Feb 27, 2023

ruclz commented Feb 28, 2023

ruclz commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruclz commented Mar 9, 2023

r-devulap commented Mar 9, 2023

ruclz commented Mar 10, 2023

r-devulap commented Mar 13, 2023

r-devulap commented Mar 13, 2023

xtangxtang commented Nov 17, 2022 •

edited

Loading

r-devulap left a comment •

edited

Loading

ruclz commented Jan 29, 2023 •

edited

Loading

ruclz commented Feb 13, 2023 •

edited

Loading

r-devulap left a comment •

edited

Loading