You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/06/10 21:51:04 UTC
[kudu-CR] [util] clean up on BlockingQueue
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16063
Change subject: [util] clean up on BlockingQueue
......................................................................
[util] clean up on BlockingQueue
Updated the interface of the BlockingQueue adding methods with the move
semantics. Switched from std::list to std::dequeue for the underlying
queue.
Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 102 insertions(+), 87 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/16063/1
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16063/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16063/3//COMMIT_MSG@10
PS3, Line 10: Switched from std::list to std::deque for the underlying
: queue.
> What's the reason for the switch?
I thought switching will make it more performant, and it turned to be exactly so according to the newly added in blocking_queue-test.
http://gerrit.cloudera.org:8080/#/c/16063/3/src/kudu/util/blocking_queue.h
File src/kudu/util/blocking_queue.h:
http://gerrit.cloudera.org:8080/#/c/16063/3/src/kudu/util/blocking_queue.h@149
PS3, Line 149: QueueStatus Put(const T& val) {
> Same here, why not wrap the r-value Put()?
Done
http://gerrit.cloudera.org:8080/#/c/16063/3/src/kudu/util/blocking_queue.h@192
PS3, Line 192: if (PREDICT_FALSE(deadline.Initialized() && MonoTime::Now() > deadline)) {
: return Status::TimedOut("");
: }
: MutexLock l(lock_);
: while (true) {
: if (PREDICT_FALSE(shutdown_)) {
: return Status::Aborted("");
: }
: if (size_ < max_size_) {
: increment_size_unlocked(val);
: queue_.emplace_back(val);
: l.Unlock();
: not_empty_.Signal();
: return Status::OK();
: }
: if (!deadline.Initialized()) {
: not_full_.Wait();
: } else if (PREDICT_FALSE(!not_full_.WaitUntil(deadline))) {
: return Status::TimedOut("");
: }
: }
> My understanding is that before this change both rvalue and const ref would
Thank you for the analysis and the explanation, Bankim!
I used the template approach for perfect forwarding. I think it's more elegant than a macro.
http://gerrit.cloudera.org:8080/#/c/16063/3/src/kudu/util/blocking_queue.h@192
PS3, Line 192: if (PREDICT_FALSE(deadline.Initialized() && MonoTime::Now() > deadline)) {
: return Status::TimedOut("");
: }
: MutexLock l(lock_);
: while (true) {
: if (PREDICT_FALSE(shutdown_)) {
: return Status::Aborted("");
: }
: if (size_ < max_size_) {
: increment_size_unlocked(val);
: queue_.emplace_back(val);
: l.Unlock();
: not_empty_.Signal();
: return Status::OK();
: }
: if (!deadline.Initialized()) {
: not_full_.Wait();
: } else if (PREDICT_FALSE(!not_full_.WaitUntil(deadline))) {
: return Status::TimedOut("");
: }
: }
> Isn't it safe to pass a const ref as an r-value? E.g. why not just wrap the
I used some more-or-less standard approach called 'perfect forwarding: https://en.cppreference.com/w/cpp/utility/forward
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 12 Jun 2020 02:43:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16063
to look at the new patch set (#7).
Change subject: [util] improved performance of BlockingQueue
......................................................................
[util] improved performance of BlockingQueue
Updated the interface of the BlockingQueue adding methods with the move
semantics. Switched from std::list to std::deque for the underlying
queue. Other minor cleanup.
This update provides measurable performance boost for BlockingQueue
as per BlockingQueueTest.MultiThreadPerf scenario of the
blocking_queue-test. I ran the following 3 times on the same
machine (RELEASE build):
./bin/blocking_queue-test --gtest_filter='BlockingQueueTest.MultiThreadPerf' --num_blocking_writers=4 --num_blocking_readers=4 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 238495 calls/sec
BlockingPut() rate: 238495 calls/sec
total Blocking{Get,Put}() rate: 476990 calls/sec
BlockingGet() rate: 223278 calls/sec
BlockingPut() rate: 223278 calls/sec
total Blocking{Get,Put}() rate: 446556 calls/sec
BlockingGet() rate: 227804 calls/sec
BlockingPut() rate: 227804 calls/sec
total Blocking{Get,Put}() rate: 455608 calls/sec
After this patch:
BlockingGet() rate: 328264 calls/sec
BlockingPut() rate: 328264 calls/sec
total Blocking{Get,Put}() rate: 656528 calls/sec
BlockingGet() rate: 312607.4 calls/sec
BlockingPut() rate: 312607.4 calls/sec
total Blocking{Get,Put}() rate: 625214.8 calls/sec
BlockingGet() rate: 292966.4 calls/sec
BlockingPut() rate: 292966.4 calls/sec
total Blocking{Get,Put}() rate: 585932.8 calls/sec
========================================================================
The case of non-symmetric writes/reads now looks more perfromant as well:
./bin/blocking_queue-test --gtest_filter='*BlockingQueueMultiThreadPerfTest*' --num_blocking_writers=2 --num_blocking_readers=1 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 399823.8 calls/sec
BlockingPut() rate: 399823.8 calls/sec
total Blocking{Get,Put}() rate: 799647.6 calls/sec
After this patch:
BlockingGet() rate: 640630.6 calls/sec
BlockingPut() rate: 640630.6 calls/sec
total Blocking{Get,Put}() rate: 1281261.2 calls/sec
========================================================================
In addition, I ran more synthetic mt-log-test and the results show
slight improvement as well (at least, the performance didn't degrade):
./bin/mt-log-test --num_writer_threads=1 --num_batches_per_thread=1000000 --num_reader_threads=0 --num_ops_per_batch_avg=1 --verify_log=false --gtest_filter='MultiThreadedLogTest.TestAppends'
Before this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.309s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.899s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.012s user 0.000s sys 0.001s
After this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.964s user 0.001s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.513s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.991s user 0.000s sys 0.000s
Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 77 insertions(+), 88 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/16063/7
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16063
to look at the new patch set (#8).
Change subject: [util] improved performance of BlockingQueue
......................................................................
[util] improved performance of BlockingQueue
Updated the interface of the BlockingQueue adding methods with the move
semantics. Switched from std::list to std::deque for the underlying
queue. Other minor cleanup.
This update provides measurable performance boost for BlockingQueue
as per BlockingQueueTest.MultiThreadPerf scenario of the
blocking_queue-test. I ran the following 3 times on the same
machine (RELEASE build):
./bin/blocking_queue-test --gtest_filter='BlockingQueueTest.MultiThreadPerf' --num_blocking_writers=4 --num_blocking_readers=4 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 238495 calls/sec
BlockingPut() rate: 238495 calls/sec
total Blocking{Get,Put}() rate: 476990 calls/sec
BlockingGet() rate: 223278 calls/sec
BlockingPut() rate: 223278 calls/sec
total Blocking{Get,Put}() rate: 446556 calls/sec
BlockingGet() rate: 227804 calls/sec
BlockingPut() rate: 227804 calls/sec
total Blocking{Get,Put}() rate: 455608 calls/sec
After this patch:
BlockingGet() rate: 328264 calls/sec
BlockingPut() rate: 328264 calls/sec
total Blocking{Get,Put}() rate: 656528 calls/sec
BlockingGet() rate: 312607.4 calls/sec
BlockingPut() rate: 312607.4 calls/sec
total Blocking{Get,Put}() rate: 625214.8 calls/sec
BlockingGet() rate: 292966.4 calls/sec
BlockingPut() rate: 292966.4 calls/sec
total Blocking{Get,Put}() rate: 585932.8 calls/sec
========================================================================
The case of non-symmetric writes/reads now looks more perfromant as well:
./bin/blocking_queue-test --gtest_filter='*BlockingQueueMultiThreadPerfTest*' --num_blocking_writers=2 --num_blocking_readers=1 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 399823.8 calls/sec
BlockingPut() rate: 399823.8 calls/sec
total Blocking{Get,Put}() rate: 799647.6 calls/sec
After this patch:
BlockingGet() rate: 640630.6 calls/sec
BlockingPut() rate: 640630.6 calls/sec
total Blocking{Get,Put}() rate: 1281261.2 calls/sec
========================================================================
In addition, I ran more synthetic mt-log-test and the results show
slight improvement as well (at least, the performance didn't degrade):
./bin/mt-log-test --num_writer_threads=1 --num_batches_per_thread=1000000 --num_reader_threads=0 --num_ops_per_batch_avg=1 --verify_log=false --gtest_filter='MultiThreadedLogTest.TestAppends'
Before this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.309s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.899s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.012s user 0.000s sys 0.001s
After this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.964s user 0.001s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.513s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.991s user 0.000s sys 0.000s
Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 78 insertions(+), 88 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/16063/8
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue
......................................................................
[util] improved performance of BlockingQueue
Updated the interface of the BlockingQueue adding methods with the move
semantics. Switched from std::list to std::deque for the underlying
queue. Other minor cleanup.
This update provides measurable performance boost for BlockingQueue
as per BlockingQueueTest.MultiThreadPerf scenario of the
blocking_queue-test. I ran the following 3 times on the same
machine (RELEASE build):
./bin/blocking_queue-test --gtest_filter='BlockingQueueTest.MultiThreadPerf' --num_blocking_writers=4 --num_blocking_readers=4 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 238495 calls/sec
BlockingPut() rate: 238495 calls/sec
total Blocking{Get,Put}() rate: 476990 calls/sec
BlockingGet() rate: 223278 calls/sec
BlockingPut() rate: 223278 calls/sec
total Blocking{Get,Put}() rate: 446556 calls/sec
BlockingGet() rate: 227804 calls/sec
BlockingPut() rate: 227804 calls/sec
total Blocking{Get,Put}() rate: 455608 calls/sec
After this patch:
BlockingGet() rate: 328264 calls/sec
BlockingPut() rate: 328264 calls/sec
total Blocking{Get,Put}() rate: 656528 calls/sec
BlockingGet() rate: 312607.4 calls/sec
BlockingPut() rate: 312607.4 calls/sec
total Blocking{Get,Put}() rate: 625214.8 calls/sec
BlockingGet() rate: 292966.4 calls/sec
BlockingPut() rate: 292966.4 calls/sec
total Blocking{Get,Put}() rate: 585932.8 calls/sec
========================================================================
The case of non-symmetric writes/reads now looks more perfromant as well:
./bin/blocking_queue-test --gtest_filter='*BlockingQueueMultiThreadPerfTest*' --num_blocking_writers=2 --num_blocking_readers=1 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 399823.8 calls/sec
BlockingPut() rate: 399823.8 calls/sec
total Blocking{Get,Put}() rate: 799647.6 calls/sec
After this patch:
BlockingGet() rate: 640630.6 calls/sec
BlockingPut() rate: 640630.6 calls/sec
total Blocking{Get,Put}() rate: 1281261.2 calls/sec
========================================================================
In addition, I ran more synthetic mt-log-test and the results show
slight improvement as well (at least, the performance didn't degrade):
./bin/mt-log-test --num_writer_threads=1 --num_batches_per_thread=1000000 --num_reader_threads=0 --num_ops_per_batch_avg=1 --verify_log=false --gtest_filter='MultiThreadedLogTest.TestAppends'
Before this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.309s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.899s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.012s user 0.000s sys 0.001s
After this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.964s user 0.001s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.513s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.991s user 0.000s sys 0.000s
Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Reviewed-on: http://gerrit.cloudera.org:8080/16063
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 78 insertions(+), 88 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16063/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16063/5//COMMIT_MSG@20
PS5, Line 20: Before this patch:
Nice improvement! I'm curious: did you get a sense for which of the deque vs move improvements was the major contributing factor? They seem to be in the same spirit of reducing unfavorable heap allocations at least.
http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:
http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/consensus/log.cc@845
PS5, Line 845: entry_batch_raw
If we've already transferred ownership of the unique_ptr to the blocking queue, won't the failed BlockingPut() call have already destructed this?
Oh, though I suppose this is only used for identification of the trace event. Probably worth a comment acknowledging it, since upon first glance it might look like we're using state we shouldn't be.
http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/util/blocking_queue.h
File src/kudu/util/blocking_queue.h:
http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/util/blocking_queue.h@145
PS5, Line 145: #if 0
Remove?
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 12 Jun 2020 03:21:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16063/3/src/kudu/util/blocking_queue.h
File src/kudu/util/blocking_queue.h:
http://gerrit.cloudera.org:8080/#/c/16063/3/src/kudu/util/blocking_queue.h@149
PS3, Line 149: QueueStatus Put(const T& val) {
Same here, why not wrap the r-value Put()?
http://gerrit.cloudera.org:8080/#/c/16063/3/src/kudu/util/blocking_queue.h@192
PS3, Line 192: if (PREDICT_FALSE(deadline.Initialized() && MonoTime::Now() > deadline)) {
: return Status::TimedOut("");
: }
: MutexLock l(lock_);
: while (true) {
: if (PREDICT_FALSE(shutdown_)) {
: return Status::Aborted("");
: }
: if (size_ < max_size_) {
: increment_size_unlocked(val);
: queue_.emplace_back(val);
: l.Unlock();
: not_empty_.Signal();
: return Status::OK();
: }
: if (!deadline.Initialized()) {
: not_full_.Wait();
: } else if (PREDICT_FALSE(!not_full_.WaitUntil(deadline))) {
: return Status::TimedOut("");
: }
: }
Isn't it safe to pass a const ref as an r-value? E.g. why not just wrap the below BlockingPut()?
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jun 2020 13:03:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16063/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16063/5//COMMIT_MSG@20
PS5, Line 20: Before this patch:
> As for the blocking_queue-test, I don't think the move improvement was the
Ack
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 12 Jun 2020 05:25:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue
......................................................................
Patch Set 8: Code-Review+2
Carrying Andrew's +2 from PS7.
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 12 Jun 2020 07:30:17 +0000
Gerrit-HasComments: No
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16063/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16063/1//COMMIT_MSG@10
PS1, Line 10: semantics. Switched from std::list to std::dequeue for the underlying
> actually it looks like it's onyl really used by Log in a perf-sensitive spo
Yep, it's used in Log (and in SubprocessServer, but the latter isn't performance-sensitive as I can see).
http://gerrit.cloudera.org:8080/#/c/16063/1//COMMIT_MSG@10
PS1, Line 10: semantics. Switched from std::list to std::dequeue for the underlying
> I think BlockingQueue is used in a bunch of high-throughput scenarios. I'm
It's a very good point, thank you! Indeed, I added a performance test in https://gerrit.cloudera.org/#/c/16064/ and it claims significant performance improvement.
I added more details into the commit message.
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jun 2020 04:25:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16063
to look at the new patch set (#6).
Change subject: [util] improved performance of BlockingQueue
......................................................................
[util] improved performance of BlockingQueue
Updated the interface of the BlockingQueue adding methods with the move
semantics. Switched from std::list to std::deque for the underlying
queue. Other minor cleanup.
This update provides measurable performance boost for BlockingQueue
as per BlockingQueueTest.MultiThreadPerf scenario of the
blocking_queue-test. I ran the following 3 times on the same
machine (RELEASE build):
./bin/blocking_queue-test --gtest_filter='BlockingQueueTest.MultiThreadPerf' --num_blocking_writers=4 --num_blocking_readers=4 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 238495 calls/sec
BlockingPut() rate: 238495 calls/sec
total Blocking{Get,Put}() rate: 476990 calls/sec
BlockingGet() rate: 223278 calls/sec
BlockingPut() rate: 223278 calls/sec
total Blocking{Get,Put}() rate: 446556 calls/sec
BlockingGet() rate: 227804 calls/sec
BlockingPut() rate: 227804 calls/sec
total Blocking{Get,Put}() rate: 455608 calls/sec
After this patch:
BlockingGet() rate: 328264 calls/sec
BlockingPut() rate: 328264 calls/sec
total Blocking{Get,Put}() rate: 656528 calls/sec
BlockingGet() rate: 312607.4 calls/sec
BlockingPut() rate: 312607.4 calls/sec
total Blocking{Get,Put}() rate: 625214.8 calls/sec
BlockingGet() rate: 292966.4 calls/sec
BlockingPut() rate: 292966.4 calls/sec
total Blocking{Get,Put}() rate: 585932.8 calls/sec
========================================================================
The case of non-symmetric writes/reads now looks more perfromant as well:
./bin/blocking_queue-test --gtest_filter='*BlockingQueueMultiThreadPerfTest*' --num_blocking_writers=2 --num_blocking_readers=1 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 399823.8 calls/sec
BlockingPut() rate: 399823.8 calls/sec
total Blocking{Get,Put}() rate: 799647.6 calls/sec
After this patch:
BlockingGet() rate: 640630.6 calls/sec
BlockingPut() rate: 640630.6 calls/sec
total Blocking{Get,Put}() rate: 1281261.2 calls/sec
========================================================================
In addition, I ran more synthetic mt-log-test and the results show
slight improvement as well (at least, the performance didn't degrade):
./bin/mt-log-test --num_writer_threads=1 --num_batches_per_thread=1000000 --num_reader_threads=0 --num_ops_per_batch_avg=1 --verify_log=false --gtest_filter='MultiThreadedLogTest.TestAppends'
Before this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.309s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.899s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.012s user 0.000s sys 0.001s
After this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.964s user 0.001s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.513s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.991s user 0.000s sys 0.000s
Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 111 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/16063/6
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16063
to look at the new patch set (#4).
Change subject: [util] improved performance of BlockingQueue
......................................................................
[util] improved performance of BlockingQueue
Updated the interface of the BlockingQueue adding methods with the move
semantics. Switched from std::list to std::deque for the underlying
queue. Other minor cleanup.
This update provides measurable performance boost for BlockingQueue
as per BlockingQueueTest.MultiThreadPerf scenario of the
blocking_queue-test. I ran the following 3 times on the same
machine (RELEASE build):
./bin/blocking_queue-test --gtest_filter='BlockingQueueTest.MultiThreadPerf' --num_blocking_writers=4 --num_blocking_readers=4 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 238495 calls/sec
BlockingPut() rate: 238495 calls/sec
total Blocking{Get,Put}() rate: 476990 calls/sec
BlockingGet() rate: 223278 calls/sec
BlockingPut() rate: 223278 calls/sec
total Blocking{Get,Put}() rate: 446556 calls/sec
BlockingGet() rate: 227804 calls/sec
BlockingPut() rate: 227804 calls/sec
total Blocking{Get,Put}() rate: 455608 calls/sec
After this patch:
BlockingGet() rate: 328264 calls/sec
BlockingPut() rate: 328264 calls/sec
total Blocking{Get,Put}() rate: 656528 calls/sec
BlockingGet() rate: 312607.4 calls/sec
BlockingPut() rate: 312607.4 calls/sec
total Blocking{Get,Put}() rate: 625214.8 calls/sec
BlockingGet() rate: 292966.4 calls/sec
BlockingPut() rate: 292966.4 calls/sec
total Blocking{Get,Put}() rate: 585932.8 calls/sec
========================================================================
In addition, I ran more synthetic mt-log-test and the results show
slight improvement as well (at least, the performance didn't degrade):
./bin/mt-log-test --num_writer_threads=1 --num_batches_per_thread=1000000 --num_reader_threads=0 --num_ops_per_batch_avg=1 --verify_log=false --gtest_filter='MultiThreadedLogTest.TestAppends'
Before this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.309s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.899s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.012s user 0.000s sys 0.001s
After this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.964s user 0.001s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.513s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.991s user 0.000s sys 0.000s
Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 168 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/16063/4
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16063
to look at the new patch set (#3).
Change subject: [util] improved performance of BlockingQueue
......................................................................
[util] improved performance of BlockingQueue
Updated the interface of the BlockingQueue adding methods with the move
semantics. Switched from std::list to std::deque for the underlying
queue. Other minor cleanup.
This update provides measurable performance boost for BlockingQueue
as per BlockingQueueTest.MultiThreadPerf scenario of the
blocking_queue-test. I ran the following 3 times on the same
machine (RELEASE build):
./bin/blocking_queue-test --gtest_filter='BlockingQueueTest.MultiThreadPerf' --num_blocking_writers=4 --num_blocking_readers=4 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 238495 calls/sec
BlockingPut() rate: 238495 calls/sec
total Blocking{Get,Put}() rate: 476990 calls/sec
BlockingGet() rate: 223278 calls/sec
BlockingPut() rate: 223278 calls/sec
total Blocking{Get,Put}() rate: 446556 calls/sec
BlockingGet() rate: 227804 calls/sec
BlockingPut() rate: 227804 calls/sec
total Blocking{Get,Put}() rate: 455608 calls/sec
After this patch:
BlockingGet() rate: 328264 calls/sec
BlockingPut() rate: 328264 calls/sec
total Blocking{Get,Put}() rate: 656528 calls/sec
BlockingGet() rate: 312607.4 calls/sec
BlockingPut() rate: 312607.4 calls/sec
total Blocking{Get,Put}() rate: 625214.8 calls/sec
BlockingGet() rate: 292966.4 calls/sec
BlockingPut() rate: 292966.4 calls/sec
total Blocking{Get,Put}() rate: 585932.8 calls/sec
========================================================================
In addition, I ran more synthetic mt-log-test and the results show
slight improvement as well (at least, the performance didn't degrade):
./bin/mt-log-test --num_writer_threads=1 --num_batches_per_thread=1000000 --num_reader_threads=0 --num_ops_per_batch_avg=1 --verify_log=false --gtest_filter='MultiThreadedLogTest.TestAppends'
Before this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.309s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.899s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.012s user 0.000s sys 0.001s
After this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.964s user 0.001s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.513s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.991s user 0.000s sys 0.000s
Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 102 insertions(+), 88 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/16063/3
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16063/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16063/5//COMMIT_MSG@20
PS5, Line 20: Before this patch:
> Nice improvement! I'm curious: did you get a sense for which of the deque v
As for the blocking_queue-test, I don't think the move improvement was the main contributing factor. The test uses uin64_t type as the element type, so the methods used there are const reference ones, which existed before the patch.
In addition, I verified that re-jiggling of the sequence sending/broadcasting on condition variables and releasing the lock is not crucial either (I returned std::list back and ran the test, and the results looked exactly like the pre-patch ones).
http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:
http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/consensus/log.cc@845
PS5, Line 845: entry_batch_raw
> If we've already transferred ownership of the unique_ptr to the blocking qu
Right, here the pointer servers as ID for the trace, and in the trace it's represented as uint64_t/uintptr_t (basically, the address).
I added a comment.
http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/util/blocking_queue.h
File src/kudu/util/blocking_queue.h:
http://gerrit.cloudera.org:8080/#/c/16063/5/src/kudu/util/blocking_queue.h@145
PS5, Line 145: #if 0
> Remove?
Done
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 12 Jun 2020 05:06:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16063
to look at the new patch set (#2).
Change subject: [util] improved performance of BlockingQueue
......................................................................
[util] improved performance of BlockingQueue
Updated the interface of the BlockingQueue adding methods with the move
semantics. Switched from std::list to std::deque for the underlying
queue. Other minor cleanup.
This update provides measurable performance boost for BlockingQueue
as per BlockingQueueTest.MultiThreadPerf scenario of the
blocking_queue-test. I ran the following 3 times on the same
machine (RELEASE build):
./bin/blocking_queue-test --gtest_filter='BlockingQueueTest.MultiThreadPerf' --num_blocking_writers=4 --num_blocking_readers=4 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 238495 calls/sec
BlockingPut() rate: 238495 calls/sec
total Blocking{Get,Put}() rate: 476990 calls/sec
BlockingGet() rate: 223278 calls/sec
BlockingPut() rate: 223278 calls/sec
total Blocking{Get,Put}() rate: 446556 calls/sec
BlockingGet() rate: 227804 calls/sec
BlockingPut() rate: 227804 calls/sec
total Blocking{Get,Put}() rate: 455608 calls/sec
After this patch:
BlockingGet() rate: 328264 calls/sec
BlockingPut() rate: 328264 calls/sec
total Blocking{Get,Put}() rate: 656528 calls/sec
BlockingGet() rate: 312607.4 calls/sec
BlockingPut() rate: 312607.4 calls/sec
total Blocking{Get,Put}() rate: 625214.8 calls/sec
BlockingGet() rate: 292966.4 calls/sec
BlockingPut() rate: 292966.4 calls/sec
total Blocking{Get,Put}() rate: 585932.8 calls/sec
========================================================================
In addition, I ran more synthetic mt-log-test and the results show
slight improvement as well (at least, the performance didn't degrade):
./bin/mt-log-test --num_writer_threads=1 --num_batches_per_thread=1000000 --num_reader_threads=0 --num_ops_per_batch_avg=1 --verify_log=false --gtest_filter='MultiThreadedLogTest.TestAppends'
Before this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.309s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.899s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.012s user 0.000s sys 0.001s
After this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.964s user 0.001s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.513s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.991s user 0.000s sys 0.000s
Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 102 insertions(+), 87 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/16063/2
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16063
to look at the new patch set (#5).
Change subject: [util] improved performance of BlockingQueue
......................................................................
[util] improved performance of BlockingQueue
Updated the interface of the BlockingQueue adding methods with the move
semantics. Switched from std::list to std::deque for the underlying
queue. Other minor cleanup.
This update provides measurable performance boost for BlockingQueue
as per BlockingQueueTest.MultiThreadPerf scenario of the
blocking_queue-test. I ran the following 3 times on the same
machine (RELEASE build):
./bin/blocking_queue-test --gtest_filter='BlockingQueueTest.MultiThreadPerf' --num_blocking_writers=4 --num_blocking_readers=4 --num_non_blocking_writers=0
Before this patch:
BlockingGet() rate: 238495 calls/sec
BlockingPut() rate: 238495 calls/sec
total Blocking{Get,Put}() rate: 476990 calls/sec
BlockingGet() rate: 223278 calls/sec
BlockingPut() rate: 223278 calls/sec
total Blocking{Get,Put}() rate: 446556 calls/sec
BlockingGet() rate: 227804 calls/sec
BlockingPut() rate: 227804 calls/sec
total Blocking{Get,Put}() rate: 455608 calls/sec
After this patch:
BlockingGet() rate: 328264 calls/sec
BlockingPut() rate: 328264 calls/sec
total Blocking{Get,Put}() rate: 656528 calls/sec
BlockingGet() rate: 312607.4 calls/sec
BlockingPut() rate: 312607.4 calls/sec
total Blocking{Get,Put}() rate: 625214.8 calls/sec
BlockingGet() rate: 292966.4 calls/sec
BlockingPut() rate: 292966.4 calls/sec
total Blocking{Get,Put}() rate: 585932.8 calls/sec
========================================================================
In addition, I ran more synthetic mt-log-test and the results show
slight improvement as well (at least, the performance didn't degrade):
./bin/mt-log-test --num_writer_threads=1 --num_batches_per_thread=1000000 --num_reader_threads=0 --num_ops_per_batch_avg=1 --verify_log=false --gtest_filter='MultiThreadedLogTest.TestAppends'
Before this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.309s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.899s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 15.012s user 0.000s sys 0.001s
After this patch:
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.964s user 0.001s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.513s user 0.000s sys 0.000s
Time spent inserting 1000000 batches(1 threads, 1000000 per-thread): real 14.991s user 0.000s sys 0.000s
Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 109 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/16063/5
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] improved performance of BlockingQueue
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] improved performance of BlockingQueue
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16063/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16063/3//COMMIT_MSG@10
PS3, Line 10: Switched from std::list to std::deque for the underlying
: queue.
What's the reason for the switch?
http://gerrit.cloudera.org:8080/#/c/16063/3/src/kudu/util/blocking_queue.h
File src/kudu/util/blocking_queue.h:
http://gerrit.cloudera.org:8080/#/c/16063/3/src/kudu/util/blocking_queue.h@192
PS3, Line 192: if (PREDICT_FALSE(deadline.Initialized() && MonoTime::Now() > deadline)) {
: return Status::TimedOut("");
: }
: MutexLock l(lock_);
: while (true) {
: if (PREDICT_FALSE(shutdown_)) {
: return Status::Aborted("");
: }
: if (size_ < max_size_) {
: increment_size_unlocked(val);
: queue_.emplace_back(val);
: l.Unlock();
: not_empty_.Signal();
: return Status::OK();
: }
: if (!deadline.Initialized()) {
: not_full_.Wait();
: } else if (PREDICT_FALSE(!not_full_.WaitUntil(deadline))) {
: return Status::TimedOut("");
: }
: }
> Isn't it safe to pass a const ref as an r-value? E.g. why not just wrap the
My understanding is that before this change both rvalue and const ref would bind to this function and now the rvalue will bind to the rvalue specific function. So earlier a copy will be created for both rvalue and const ref and with this implementation no copy for rvalue.
Having said that your concern appears more about how do we reduce code duplication across these functions.
Though a rvalue will bind to a const ref, I don't think vice-versa is true. So invoking const ref function from rvalue function will work but lose the benefit of avoiding the copy(the entire point of adding rvalue variant function). Wrapping the other way round won't work.
Perhaps using a macro will help avoid code duplication.
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jun 2020 18:47:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] clean up on BlockingQueue
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] clean up on BlockingQueue
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16063/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16063/1//COMMIT_MSG@10
PS1, Line 10: semantics. Switched from std::list to std::dequeue for the underlying
> I think BlockingQueue is used in a bunch of high-throughput scenarios. I'm
actually it looks like it's onyl really used by Log in a perf-sensitive spot. Should still probably validate that this is at least perf-neutral if not beneficial
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 10 Jun 2020 22:01:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] clean up on BlockingQueue
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16063 )
Change subject: [util] clean up on BlockingQueue
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16063/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16063/1//COMMIT_MSG@10
PS1, Line 10: semantics. Switched from std::list to std::dequeue for the underlying
I think BlockingQueue is used in a bunch of high-throughput scenarios. I'm not sure if Deque is better/worse than List but I think such a change should probably include some benchmarking and not sneak into "clean up"
--
To view, visit http://gerrit.cloudera.org:8080/16063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie80620e5e86cd72c29320096dcdcc712eea1b0f2
Gerrit-Change-Number: 16063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 10 Jun 2020 22:00:08 +0000
Gerrit-HasComments: Yes