[feature] Supports bthread scanner #16031

Xiaoccer · 2023-01-17T14:54:32Z

Proposed changes

Issue Number: close #xxx

Problem summary

Add bthread to separate the logic of IO and computation when executing the OlapScanner, which can speed up to access those sql that are already cached.

In OlapScanner's execution chain, change std::mutex/std::condition_variable to bthread::Mutex/bthread::ConditionVariable
Add class of AsyncIO to separate task from bthread to pthread
Change the usage of reader's/filesytem's interface when using bthread

Performance test

1be+1fe
base: commit_id-bd2280b4ce702e24cc31ca1d379aeaf6f00ce69c

ssbf100 benchmark

sql	base(s)	bthread(s)
q1.1	0.92	0.904
q1.2	1.122	1.126
q1.3	0.052	0.052
q2.1	12.041	12.072
q2.2	0.741	0.705
q2.3	0.635	0.671
q3.1	3.492	3.422
q3.2	0.46	0.47
q3.3	0.649	0.679
q3.4	0.089	0.087
q4.1	4.358	4.685
q4.2	0.519	0.572
q4.3	0.457	0.463

The performance of using bthread and using pthread is almost the same.

cached read

Description: First execute q1.1.sql to cache data, and then execute q1.1.sql、q2.1.sql and q4.1.sql concurrently.

sql	base(s)	bthread(s)
first time q1.1	2.673	2.721
second time (cached) q1.1	13.441	0.206
first time q2.1	53.846	53.793
first time q4.1	53.864	53.903

When using bthread, If the data of sql has been cached, the result of sql can be returned fast without waiting the free thread of thread pool.

Checklist(Required)

Does it affect the original behavior:
- Yes
- No
- I don't know
Has unit tests been added:
- Yes
- No
- No Need
Has document been added or modified:
- Yes
- No
- No Need
Does it need to update dependencies:
- Yes
- No
Are there any changes that cannot be rolled back:
- Yes (If Yes, please explain WHY)
- No

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

github-actions · 2023-01-17T14:55:26Z

`sh-checker report`

To get the full details, please check in the job output.

shellcheck errors


'shellcheck ' returned error 1 finding the following syntactical issues:

----------

In build.sh line 428:
	-DUSE_BTHREAD_SCANNER=${USE_BTHREAD_SCANNER} \
                              ^--------------------^ SC2248 (style): Prefer double quoting even when variables don't contain special characters.

Did you mean: 
	-DUSE_BTHREAD_SCANNER="${USE_BTHREAD_SCANNER}" \

For more information:
  https://www.shellcheck.net/wiki/SC2248 -- Prefer double quoting even when v...
----------

You can address the above issues in one of three ways:
1. Manually correct the issue in the offending shell script;
2. Disable specific issues by adding the comment:
  # shellcheck disable=NNNN
above the line that contains the issue, where NNNN is the error code;
3. Add '-e NNNN' to the SHELLCHECK_OPTS setting in your .yml action file.

shfmt errors


'shfmt ' returned error 1 finding the following formatting issues:

----------
--- build.sh.orig
+++ build.sh
@@ -425,7 +425,7 @@
         -DUSE_DWARF="${USE_DWARF}" \
         -DUSE_MEM_TRACKER="${USE_MEM_TRACKER}" \
         -DUSE_JEMALLOC="${USE_JEMALLOC}" \
-	-DUSE_BTHREAD_SCANNER=${USE_BTHREAD_SCANNER} \
+        -DUSE_BTHREAD_SCANNER=${USE_BTHREAD_SCANNER} \
         -DSTRICT_MEMORY_USE="${STRICT_MEMORY_USE}" \
         -DENABLE_STACKTRACE="${ENABLE_STACKTRACE}" \
         -DUSE_AVX2="${USE_AVX2}" \
----------

You can reformat the above files to meet shfmt's requirements by typing:

  shfmt  -w filename

github-actions

clang-tidy made some suggestions

github-actions · 2023-01-17T15:04:08Z

be/src/util/async_io.h

+    PriorityThreadPool* _io_thread_pool = nullptr;
+    PriorityThreadPool* _remote_thread_pool = nullptr;
+
+private:


warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]

Suggested change

private:

be/src/util/async_io.h:79: previously declared here

private: ^

github-actions · 2023-01-17T15:04:08Z

be/src/util/once.h

+        if (!_has_called.load(std::memory_order_acquire)) {
+            do {
+                std::lock_guard l(_mutex);
+                if (_has_called.load(std::memory_order_acquire)) break;


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

if (_has_called.load(std::memory_order_acquire)) break;

if (_has_called.load(std::memory_order_acquire)) { break;

}

github-actions · 2023-01-17T15:04:09Z

be/src/util/priority_work_stealing_thread_pool.hpp

    // Blocks until the work queue is empty, and then calls shutdown to stop the worker
    // threads and Join to wait until they are finished.
    // Any work Offer()'ed during DrainAndshutdown may or may not be processed.
    void drain_and_shutdown() override {


warning: only virtual member functions can be marked 'override' [clang-diagnostic-error]

Suggested change

void drain_and_shutdown() override {

void drain_and_shutdown() {

github-actions · 2023-01-17T15:04:09Z

be/src/util/priority_work_stealing_thread_pool.hpp

    void drain_and_shutdown() override {
        {
-            std::unique_lock<std::mutex> l(_lock);
+            std::unique_lock l(_lock);


warning: use of undeclared identifier '_lock'; did you mean 'clock'? [clang-diagnostic-error]

Suggested change

std::unique_lock l(_lock);

std::unique_lock l(clock);

/usr/include/time.h:71: 'clock' declared here

extern clock_t clock (void) __THROW; ^

github-actions · 2023-01-17T15:04:09Z

be/src/util/priority_work_stealing_thread_pool.hpp

-            std::unique_lock<std::mutex> l(_lock);
+            std::unique_lock l(_lock);
            while (get_queue_size() != 0) {
                _empty_cv.wait(l);


warning: use of undeclared identifier '_empty_cv' [clang-diagnostic-error]

_empty_cv.wait(l); ^

hello-stephen · 2023-01-17T20:15:34Z

TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 34.89 seconds
load time: 469 seconds
storage size: 17171040497 Bytes
https://doris-community-test-1308700295.cos.ap-hongkong.myqcloud.com/tmp/20230208044414_clickbench_pr_91881.html

github-actions · 2023-01-18T03:47:03Z

`sh-checker report`

To get the full details, please check in the job output.

shellcheck errors


'shellcheck ' returned error 1 finding the following syntactical issues:

----------

In build.sh line 428:
	-DUSE_BTHREAD_SCANNER=${USE_BTHREAD_SCANNER} \
                              ^--------------------^ SC2248 (style): Prefer double quoting even when variables don't contain special characters.

Did you mean: 
	-DUSE_BTHREAD_SCANNER="${USE_BTHREAD_SCANNER}" \

For more information:
  https://www.shellcheck.net/wiki/SC2248 -- Prefer double quoting even when v...
----------

You can address the above issues in one of three ways:
1. Manually correct the issue in the offending shell script;
2. Disable specific issues by adding the comment:
  # shellcheck disable=NNNN
above the line that contains the issue, where NNNN is the error code;
3. Add '-e NNNN' to the SHELLCHECK_OPTS setting in your .yml action file.

shfmt errors


'shfmt ' returned error 1 finding the following formatting issues:

----------
--- build.sh.orig
+++ build.sh
@@ -425,7 +425,7 @@
         -DUSE_DWARF="${USE_DWARF}" \
         -DUSE_MEM_TRACKER="${USE_MEM_TRACKER}" \
         -DUSE_JEMALLOC="${USE_JEMALLOC}" \
-	-DUSE_BTHREAD_SCANNER=${USE_BTHREAD_SCANNER} \
+        -DUSE_BTHREAD_SCANNER=${USE_BTHREAD_SCANNER} \
         -DSTRICT_MEMORY_USE="${STRICT_MEMORY_USE}" \
         -DENABLE_STACKTRACE="${ENABLE_STACKTRACE}" \
         -DUSE_AVX2="${USE_AVX2}" \
----------

You can reformat the above files to meet shfmt's requirements by typing:

  shfmt  -w filename

gavinchou · 2023-02-06T12:21:54Z

LGTM

gavinchou

LGTM

github-actions · 2023-02-07T15:14:46Z

`sh-checker report`

To get the full details, please check in the job output.

shellcheck errors

'shellcheck ' found no issues.

shfmt errors


'shfmt ' returned error 1 finding the following formatting issues:

----------
--- build.sh.orig
+++ build.sh
@@ -426,7 +426,7 @@
         -DUSE_DWARF="${USE_DWARF}" \
         -DUSE_MEM_TRACKER="${USE_MEM_TRACKER}" \
         -DUSE_JEMALLOC="${USE_JEMALLOC}" \
-	-DUSE_BTHREAD_SCANNER="${USE_BTHREAD_SCANNER}" \
+        -DUSE_BTHREAD_SCANNER="${USE_BTHREAD_SCANNER}" \
         -DSTRICT_MEMORY_USE="${STRICT_MEMORY_USE}" \
         -DENABLE_STACKTRACE="${ENABLE_STACKTRACE}" \
         -DUSE_AVX2="${USE_AVX2}" \
----------

You can reformat the above files to meet shfmt's requirements by typing:

  shfmt  -w filename

github-actions

clang-tidy made some suggestions

github-actions · 2023-02-07T15:23:24Z

be/src/io/fs/file_system_map.h

+    static FileSystemMap* instance();
+    ~FileSystemMap() = default;
+
+    void insert(ResourceId id, FileSystemSPtr fs);


warning: unknown type name 'ResourceId' [clang-diagnostic-error]

void insert(ResourceId id, FileSystemSPtr fs); ^

github-actions · 2023-02-07T15:23:25Z

be/src/io/fs/file_system_map.h

+    void insert(ResourceId id, FileSystemSPtr fs);
+
+    // If `id` is not in `_map`, return nullptr.
+    FileSystemSPtr get(const ResourceId& id);


warning: unknown type name 'ResourceId' [clang-diagnostic-error]

FileSystemSPtr get(const ResourceId& id); ^

github-actions · 2023-02-07T15:23:25Z

be/src/io/fs/file_system_map.h

+private:
+    FileSystemMap() = default;
+
+private:


warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]

Suggested change

private:

be/src/io/fs/file_system_map.h:39: previously declared here

private: ^

github-actions · 2023-02-07T15:23:25Z

be/src/io/fs/file_system_map.h

+
+private:
+    doris::SharedMutex _mu;
+    std::unordered_map<ResourceId, FileSystemSPtr> _map; // GUARED_BY(_mu)


warning: use of undeclared identifier 'ResourceId' [clang-diagnostic-error]

std::unordered_map<ResourceId, FileSystemSPtr> _map; // GUARED_BY(_mu) ^

github-actions

clang-tidy made some suggestions

github-actions · 2023-02-07T16:05:57Z

be/src/util/async_io.h

+    PriorityThreadPool* _io_thread_pool = nullptr;
+    PriorityThreadPool* _remote_thread_pool = nullptr;
+
+private:


warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]

Suggested change

private:

be/src/util/async_io.h:96: previously declared here

private: ^

github-actions · 2023-02-08T15:24:59Z

PR approved by anyone and no changes requested.

dataroaring

LGTM

github-actions · 2023-02-09T02:24:45Z

PR approved by at least one committer and no changes requested.

github-actions bot added the area/vectorization label Jan 17, 2023

github-actions bot reviewed Jan 17, 2023

View reviewed changes

morningman added the dev/1.2.2 label Jan 17, 2023

adonis0147 closed this Jan 18, 2023

adonis0147 reopened this Jan 18, 2023

gavinchou reviewed Feb 7, 2023

View reviewed changes

gavinchou previously approved these changes Feb 7, 2023

View reviewed changes

[feature] Supports bthread scanner

eecbcde

Xiaoccer dismissed gavinchou’s stale review via eecbcde February 7, 2023 15:13

Xiaoccer force-pushed the support_bthread_scanner branch from 128c4d0 to eecbcde Compare February 7, 2023 15:13

github-actions bot reviewed Feb 7, 2023

View reviewed changes

format

9cd2777

github-actions bot reviewed Feb 7, 2023

View reviewed changes

fix

9bf92da

gavinchou approved these changes Feb 8, 2023

View reviewed changes

github-actions bot added the reviewed label Feb 8, 2023

dataroaring approved these changes Feb 9, 2023

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Feb 9, 2023

dataroaring merged commit 0142ef8 into apache:master Feb 9, 2023

YangShaw pushed a commit to YangShaw/doris that referenced this pull request Feb 17, 2023

[improvement](scanner) Supports bthread scanner (apache#16031)

e5d12e3

	if (_has_called.load(std::memory_order_acquire)) break;
	if (_has_called.load(std::memory_order_acquire)) { break;
	}

	void drain_and_shutdown() override {
	void drain_and_shutdown() {

[feature] Supports bthread scanner #16031

[feature] Supports bthread scanner #16031

Uh oh!

Conversation

Xiaoccer commented Jan 17, 2023

Proposed changes

Problem summary

Performance test

ssbf100 benchmark

cached read

Checklist(Required)

Further comments

Uh oh!

github-actions bot commented Jan 17, 2023

sh-checker report

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

hello-stephen commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 18, 2023

sh-checker report

Uh oh!

gavinchou commented Feb 6, 2023

Uh oh!

gavinchou left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 7, 2023

sh-checker report

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 8, 2023

Uh oh!

dataroaring left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

`sh-checker report`

hello-stephen commented Jan 17, 2023 •

edited

Loading

`sh-checker report`

`sh-checker report`