Add multithreaded sample sort and use it to sort memtx keys in secondary indexes on startup #8562

nshy · 2023-04-12T15:45:31Z

In short:

now we don't use OpenMP at all
sorting does not load TX thread so Tarantool should be responsive while building indexes
number of sort threads is limited to configuration parameter memtx_sort_threads

Closes #3389
Closes #7689
Closes #4646

coveralls · 2023-04-12T15:56:13Z

coverage: 85.872% (+0.09%) from 85.78% when pulling e02039e on nshy:add-multithread-sample-sort into 8315869
on tarantool:master.

changelogs/unreleased/gh-3389-cfg-option-memtx-sort-threads.md

denesterov · 2023-04-13T13:18:36Z

src/box/memtx_engine.cc

@@ -1412,6 +1413,9 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,
 	memtx->state = MEMTX_INITIALIZED;
 	memtx->max_tuple_size = MAX_TUPLE_SIZE;
 	memtx->force_recovery = force_recovery;
+	if (sort_threads == 0)
+		sort_threads = get_nprocs();
+	memtx->sort_threads = sort_threads;


Suggest default behavior sort_threads = max(1, (int)get_nprocs() - 1)

Try not to step on the same rake - do not consume all cores by default.

I guess it won't much help as threads number limit is added to keep others Tarantool instances running while this one is starting. Other instances number can be more then one and also Tarantool have more then one thread that can be loaded with work. I guess it is better to use introduced option to limit starting Tarantool according to the installation needs.

Yet I'll add low limit 1 (and high too) on sorting threads.

andreyaksenov

My suggestion regarding wording in a changelog:

* Added the `box.cfg.memtx_sort_threads` parameter that specifies the number of threads
  used to sort keys of secondary indexes on loading a memtx database (gh-3389).

locker

Please see a few comments. I haven't reviewed the algorithm in detail yet so there may be more.

src/lib/core/sample_sort.c

test/unit/qsort_arg.cc

src/lib/core/sample_sort.h

src/lib/core/sample_sort.c

third_party/qsort_arg.c

src/box/box.cc

src/box/lua/load_cfg.lua

src/lib/core/sample_sort.c

test/unit/qsort_arg.cc

test/unit/sort.cc

src/lib/core/sort.c

src/box/box.cc

alyapunov

Almost good for me, see several comments.

alyapunov · 2023-04-18T14:15:48Z

src/lib/core/tt_sort.c

+ * application faster if there is no much data in database (test cases in
+ * particular). Otherwise sorting will be done in threads.
+ */
+


It seems to be an excess line break.

alyapunov · 2023-04-18T14:51:52Z

changelogs/unreleased/gh-3389-cfg-option-memtx-sort-threads.md

+## feature/box
+
+* Added the `box.cfg.memtx_sort_threads` parameter that specifies the number of
+  threads used to sort keys of secondary indexes on loading a memtx database


I think we should directly state that openmp is used no more. Actually the patch is a kind of breaking change: before an advanced user can specify OMP_NUM_THREADS as some value. Now that think doesn't work anymore. Maybe we should read the variable if memtx_sort_threads is not specified, just for backward compatibility.. Perhaps under a compat option..

Done.

if (sort_threads == 0) { char *ompnum_str = getenv_safe("OMP_NUM_THREADS", NULL, 0); if (ompnum_str != NULL) { long ompnum = strtol(ompnum_str, NULL, 10); if (ompnum > 0 && ompnum <= TT_SORT_THREADS_MAX) { say_warn("OMP_NUM_THREADS is used to set number" " of sorting threads. Use cfg option" "'memtx_sort_threads' instead."); sort_threads = ompnum; } free(ompnum_str); } if (sort_threads == 0) { sort_threads = get_nprocs(); if (sort_threads < 1) sort_threads = 1; else if (sort_threads > TT_SORT_THREADS_MAX) sort_threads = TT_SORT_THREADS_MAX; } } memtx->sort_threads = sort_threads;

test/box-luatest/gh_3389_cfg_option_memtx_sort_threads_test.lua

alyapunov · 2023-04-18T15:42:04Z

src/lib/core/tt_sort.c

+ * place most left and most right boundaries at imaginary indexes `-1` and
+ * `size of splitters` respectively.
+ */
+static inline int


Nit: Usually we does not use inline in static functions within .c file.

alyapunov · 2023-04-24T15:33:07Z

src/lib/core/tt_sort.c

+ * Find bucket for element using binary search among sorted in ascending
+ * order splitters.
+ *
+ * Bucket number is `thread_count`, thus bucket boundraries (splitters) number


For me number here is misleading. I think count will be correct and sound.

According to our coding style, generally, you want your comments to tell WHAT your code does, not HOW.

In other word the comment should describe what find_bucket makes and returns. It would be a good idea (but not necessary) to state that the 0 <= return value < thread_count. This comment is needed when you use the function.

The comment about imaginary indexes is also good, but should be placed in the function body. This comment is needed for better understanding when you read the function code.

Changed to:

/** * Find bucket for element using binary search among sorted in ascending * order splitters. * * Return index of the bucket for the element. */ static int find_bucket(struct tt_sort *sort, void *elem) { /* * Bucket count is `thread_count`, thus bucket boundaries (splitters) * count is `thread_count - 1` omitting most left and most right * boundaries. Let's place most left and most right boundaries at * imaginary indexes `-1` and `size of splitters` respectively. */

I didn't write the exact assertion for the return value in the at the file level. Looks like term index should be enough.

alyapunov · 2023-04-28T11:35:59Z

src/lib/core/tt_sort.c

+ * Sample sort algorithm data.
+ */
+struct tt_sort {
+	/**


Nit: We usually use one-line comments for short descriptions. Sometimes it simplifies code observation:
/** The data being sorted. */

I tried one-liners in this structure. IMHO it reduced readability. May be it is a matter of screen size or fonts.

Used one-line comment after f2f discussion.

alyapunov · 2023-04-28T12:59:18Z

src/lib/core/tt_sort.c

+/**
+ * Data for a single sample sort worker thread.
+ */
+struct sort_worker {


A bit inconsistent name. I mean struct tt_sort and struct sort_worker.
I would name them struct sort_data and struct sort_worker, or struct tt_sort and tt_worker..

Changed to sort_data.

alyapunov · 2023-04-28T14:02:57Z

src/lib/core/tt_sort.c

+	sort.cmp_arg = cmp_arg;
+	sort.thread_count = thread_count;
+
+	if (thread_count == 1) {


This function (tt_sort) is too big. It seems that if we create a couple of helpers (for example tt_sort_one_thread) it would be much shorter and thus understandable.

nshy · 2023-05-22T15:43:54Z

DО NOT MERGE before tarantool/test-run#391.

nshy · 2023-06-08T14:52:34Z

Added references to the tickets to commit messages.
Rebased to master to fix conflict in 'test: bump test-run to new version' patch.

The algorithm runs sort in multiple threads and does not use OpenMP. It has better threads utilization right from the beginning but probably a worse constant than parallel qsort. See details in code comments. Besides sort is not performed in calling thread but instead in spawned worker threads. Calling thread yields waiting for worker threads to finish. Exception is small data size, in this case sorting is executed in calling thread saving time on spawning a thread. This should speed up test execution. This is existing behaviour of qsort_arg but data size threshold is reduced from 128000 to 1024. Part of tarantool#3389 NO_CHANGELOG=internal NO_DOC=internal

Bump test-run to new version with the following improvements: - Drop setting OMP_NUM_THREADS Part of tarantool#3389 NO_DOC=testing stuff NO_TEST=testing stuff NO_CHANGELOG=testing stuff

nshy · 2023-06-09T08:26:29Z

Next changes are done to pass full CI:

use sysconf(_SC_NPROCESSORS_ONLN) instead of get_nprocs(). The last one is GNU specific. Also add a warning on fallback to single core.

--- a/src/box/memtx_engine.cc
+++ b/src/box/memtx_engine.cc
@@ -33,7 +33,6 @@
 #include <small/quota.h>
 #include <small/small.h>
 #include <small/mempool.h>
-#include <sys/sysinfo.h>

 #include "fiber.h"
 #include "errinj.h"
@@ -1519,11 +1518,14 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,
                        free(ompnum_str);
                }
                if (sort_threads == 0) {
-                       sort_threads = get_nprocs();
-                       if (sort_threads < 1)
+                       sort_threads = sysconf(_SC_NPROCESSORS_ONLN);
+                       if (sort_threads < 1) {
+                               say_warn("Cannot get number of processors."
+                                        "Fallback to single processor.");
                                sort_threads = 1;
-                       else if (sort_threads > TT_SORT_THREADS_MAX)
+                       } else if (sort_threads > TT_SORT_THREADS_MAX) {
                                sort_threads = TT_SORT_THREADS_MAX;
+                       }
                }
        }

Fixed test/uni/tt_sort.cc build. The previous version failed to link on some OSXes. Probably it has stricter linking rules.

 create_unit_test(PREFIX tt_sort
                  SOURCES tt_sort.cc core_test_utils.c
-                 LIBRARIES misc unit box
+                 LIBRARIES unit core
 )

nshy · 2023-06-09T11:36:53Z

Fixed messages whitespaces:

diff --git a/src/box/box.cc b/src/box/box.cc
index cc61ba743..b9a5e0c66 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1675,7 +1675,7 @@ box_check_memtx_sort_threads(void)
        if (cfg_isnumber("memtx_sort_threads") &&
            (num <= 0 || num > TT_SORT_THREADS_MAX))
                tnt_raise(ClientError, ER_CFG, "memtx_sort_threads",
-                         tt_sprintf("must be greater than 0 and less than or "
+                         tt_sprintf("must be greater than 0 and less than or"
                                     " equal to %d", TT_SORT_THREADS_MAX));
 }

diff --git a/src/box/memtx_engine.cc b/src/box/memtx_engine.cc
index 49cff1f6d..6ba8eba11 100644
--- a/src/box/memtx_engine.cc
+++ b/src/box/memtx_engine.cc
@@ -1512,7 +1512,7 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,
                        if (ompnum > 0 && ompnum <= TT_SORT_THREADS_MAX) {
                                say_warn("OMP_NUM_THREADS is used to set number"
                                         " of sorting threads. Use cfg option"
-                                        "'memtx_sort_threads' instead.");
+                                        " 'memtx_sort_threads' instead.");
                                sort_threads = ompnum;
                        }
                        free(ompnum_str);
@@ -1520,8 +1520,8 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,
                if (sort_threads == 0) {
                        sort_threads = sysconf(_SC_NPROCESSORS_ONLN);
                        if (sort_threads < 1) {
-                               say_warn("Cannot get number of processors."
-                                        "Fallback to single processor.");
+                               say_warn("Cannot get number of processors. "
+                                        "Fallback to single processor.");
                                sort_threads = 1;
                        } else if (sort_threads > TT_SORT_THREADS_MAX) {
                                sort_threads = TT_SORT_THREADS_MAX;
diff --git a/test/box-luatest/gh_3389_cfg_option_memtx_sort_threads_test.lua b/test/box-luatest/gh_3389_cfg_option_memtx_sort_threads_test.lua
index 65863d87c..4b9bb8c08 100644
--- a/test/box-luatest/gh_3389_cfg_option_memtx_sort_threads_test.lua
+++ b/test/box-luatest/gh_3389_cfg_option_memtx_sort_threads_test.lua
@@ -46,7 +46,7 @@ g.test_setting_openmp_env_var = function(cg)
     t.helpers.retrying({}, function()
         local p = "OMP_NUM_THREADS is used to set number" ..
                   " of sorting threads. Use cfg option" ..
-                  "'memtx_sort_threads' instead."
+                  " 'memtx_sort_threads' instead."
         t.assert_not_equals(cg.server:grep_log(p), nil)
     end)
     cg.server:stop()

@TarantoolBot

Closes tarantool#3389 Closes tarantool#7689 Closes tarantool#4646 @TarantoolBot document Title: new box.cfg parameter memtx_sort_threads The parameter sets the number of threads used to sort keys of secondary indexes on loading memtx database. The parameter cannot be changed dynamically (as it does not make sense). Maximum value is 256, minimum is 1. Default is to use all available cores. Usage example: ``` box.cfg{memtx_sort_threads=4} ```

nshy requested a review from a team as a code owner April 12, 2023 15:45

nshy requested a review from locker April 12, 2023 15:45

nshy assigned locker and nshy and unassigned locker and nshy Apr 12, 2023

Gumix reviewed Apr 13, 2023

View reviewed changes

changelogs/unreleased/gh-3389-cfg-option-memtx-sort-threads.md Outdated Show resolved Hide resolved

denesterov reviewed Apr 13, 2023

View reviewed changes

andreyaksenov approved these changes Apr 13, 2023

View reviewed changes

locker reviewed Apr 13, 2023

View reviewed changes

locker assigned nshy Apr 13, 2023

locker reviewed Apr 14, 2023

View reviewed changes

locker removed their assignment Apr 14, 2023

nshy force-pushed the add-multithread-sample-sort branch 2 times, most recently from 7cc088c to cd3aad3 Compare April 17, 2023 08:48

nshy assigned locker and unassigned nshy Apr 17, 2023

locker reviewed Apr 17, 2023

View reviewed changes

test/unit/qsort_arg.cc Outdated Show resolved Hide resolved

test/unit/sort.cc Outdated Show resolved Hide resolved

src/lib/core/sort.c Outdated Show resolved Hide resolved

src/lib/core/sort.c Outdated Show resolved Hide resolved

src/box/box.cc Outdated Show resolved Hide resolved

locker assigned nshy and unassigned locker Apr 17, 2023

nshy added the do not merge Not ready to be merged label Apr 17, 2023

nshy force-pushed the add-multithread-sample-sort branch 2 times, most recently from a33b2f5 to 0e17a7a Compare April 17, 2023 15:27

nshy assigned locker and nshy and unassigned nshy and locker Apr 17, 2023

nshy force-pushed the add-multithread-sample-sort branch from 0e17a7a to c8cdabb Compare April 18, 2023 08:23

alyapunov requested changes Apr 28, 2023

View reviewed changes

alyapunov assigned nshy and unassigned alyapunov Apr 28, 2023

nshy force-pushed the add-multithread-sample-sort branch 3 times, most recently from 99af2a8 to 74bc106 Compare May 22, 2023 15:42

nshy assigned alyapunov and unassigned nshy May 22, 2023

alyapunov approved these changes Jun 8, 2023

View reviewed changes

alyapunov assigned nshy and unassigned alyapunov Jun 8, 2023

nshy force-pushed the add-multithread-sample-sort branch from 74bc106 to 11a963d Compare June 8, 2023 14:41

nshy removed the do not merge Not ready to be merged label Jun 8, 2023

nshy force-pushed the add-multithread-sample-sort branch from 11a963d to f3c5427 Compare June 8, 2023 14:50

nshy added the full-ci Enables all tests for a pull request label Jun 8, 2023

nshy force-pushed the add-multithread-sample-sort branch from f3c5427 to f83a2cc Compare June 9, 2023 07:21

nshy added 2 commits June 9, 2023 10:48

test: bump test-run to new version

eef6944

Bump test-run to new version with the following improvements: - Drop setting OMP_NUM_THREADS Part of tarantool#3389 NO_DOC=testing stuff NO_TEST=testing stuff NO_CHANGELOG=testing stuff

nshy force-pushed the add-multithread-sample-sort branch from f83a2cc to f3ed1ad Compare June 9, 2023 07:50

nshy assigned alyapunov and unassigned nshy Jun 9, 2023

nshy force-pushed the add-multithread-sample-sort branch from f3ed1ad to e8c81ef Compare June 9, 2023 11:35

nshy force-pushed the add-multithread-sample-sort branch from e8c81ef to e02039e Compare June 9, 2023 11:41

alyapunov merged commit 4f617b7 into tarantool:master Jun 9, 2023
86 checks passed

nshy deleted the add-multithread-sample-sort branch June 14, 2023 07:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multithreaded sample sort and use it to sort memtx keys in secondary indexes on startup #8562

Add multithreaded sample sort and use it to sort memtx keys in secondary indexes on startup #8562

nshy commented Apr 12, 2023

coveralls commented Apr 12, 2023 •

edited

denesterov Apr 13, 2023

nshy Apr 14, 2023

andreyaksenov left a comment

locker left a comment

alyapunov left a comment

alyapunov Apr 18, 2023

nshy May 15, 2023

alyapunov Apr 18, 2023

nshy May 22, 2023

alyapunov Apr 18, 2023

nshy May 15, 2023

alyapunov Apr 24, 2023

alyapunov Apr 24, 2023

nshy May 15, 2023 •

edited

alyapunov Apr 28, 2023

nshy May 15, 2023

nshy May 22, 2023

alyapunov Apr 28, 2023

nshy May 15, 2023

alyapunov Apr 28, 2023

nshy commented May 22, 2023

nshy commented Jun 8, 2023

nshy commented Jun 9, 2023

nshy commented Jun 9, 2023

Add multithreaded sample sort and use it to sort memtx keys in secondary indexes on startup #8562

Add multithreaded sample sort and use it to sort memtx keys in secondary indexes on startup #8562

Conversation

nshy commented Apr 12, 2023

coveralls commented Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyaksenov left a comment

Choose a reason for hiding this comment

locker left a comment

Choose a reason for hiding this comment

alyapunov 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nshy May 15, 2023 • edited

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

nshy commented May 22, 2023

nshy commented Jun 8, 2023

nshy commented Jun 9, 2023

nshy commented Jun 9, 2023

coveralls commented Apr 12, 2023 •

edited

nshy May 15, 2023 •

edited