You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2019/10/26 09:42:09 UTC
[kudu-CR] KUDU-2977 sharding block map to speed up tserver startup
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14555
Change subject: KUDU-2977 sharding block map to speed up tserver startup
......................................................................
KUDU-2977 sharding block map to speed up tserver startup
Separate LogBlockManager's block lock and container lock,
and sharding block map.
Time cost of LogBlockManagerTest.StartupBenchmark in old version:
I1026 17:17:30.231856 150030 log_block_manager-test.cc:1042] Time spent reopening block manager: real 1.148s user 0.092s sys 0.030s
I1026 17:17:30.798593 160153 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.062s user 0.062s sys 0.000s
I1026 17:17:30.866621 160151 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.109s user 0.068s sys 0.000s
I1026 17:17:30.949687 160154 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.192s user 0.083s sys 0.000s
I1026 17:17:31.007802 160147 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.264s user 0.058s sys 0.000s
I1026 17:17:31.103804 160149 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.339s user 0.094s sys 0.002s
I1026 17:17:31.171495 160150 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.417s user 0.068s sys 0.000s
I1026 17:17:31.252955 160148 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.502s user 0.081s sys 0.000s
I1026 17:17:31.331007 160152 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.581s user 0.078s sys 0.000s
new version:
I1026 17:15:48.425242 52360 log_block_manager-test.cc:1042] Time spent reopening block manager: real 0.945s user 0.159s sys 0.001s
I1026 17:15:49.245585 64003 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.256s user 0.162s sys 0.022s
I1026 17:15:49.323187 63997 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.315s user 0.190s sys 0.027s
I1026 17:15:49.351920 64004 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.330s user 0.213s sys 0.042s
I1026 17:15:49.377976 63999 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.341s user 0.229s sys 0.032s
I1026 17:15:49.380051 64002 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.336s user 0.208s sys 0.034s
I1026 17:15:49.387194 63998 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.330s user 0.215s sys 0.040s
I1026 17:15:49.399719 64001 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.317s user 0.206s sys 0.034s
I1026 17:15:49.434571 64000 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.322s user 0.187s sys 0.061s
Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 145 insertions(+), 90 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/14555/1
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
Patch Set 5:
> Patch Set 5:
>
> The TSAN failure looks real but I don't know what's causing it.
I read some spp.h code, found that it's a problem of spare-map. Code fragment below (thirdparty/installed/common/include/sparsepp/spp.h:1082):
static uint32_t _sizing(uint32_t n)
{
#if !defined(SPP_ALLOC_SZ) || (SPP_ALLOC_SZ == 0)
// aggressive allocation first, then decreasing as sparsegroups fill up
// --------------------------------------------------------------------
static uint8_t s_alloc_batch_sz[SPP_GROUP_SIZE] = { 0 };
if (!s_alloc_batch_sz[0])
{
// 32 bit bitmap
// ........ .... .... .. .. .. .. . . . . . . . .
// 8 12 16 18 20 22 24 25 26 ... 32
// ------------------------------------------------------
uint8_t group_sz = SPP_GROUP_SIZE / 4;
uint8_t group_start_alloc = SPP_GROUP_SIZE / 8; //4;
uint8_t alloc_sz = group_start_alloc;
for (int i=0; i<4; ++i)
{
for (int j=0; j<group_sz; ++j)
{
if (j && j % group_start_alloc == 0)
alloc_sz += group_start_alloc;
s_alloc_batch_sz[i * group_sz + j] = alloc_sz; // data race here!!!
}
if (group_start_alloc > 2)
group_start_alloc /= 2;
alloc_sz += group_start_alloc;
}
}
return n ? static_cast<uint32_t>(s_alloc_batch_sz[n-1]) : 0; // more aggressive alloc at the beginning
#elif (SPP_ALLOC_SZ == 1)
// use as little memory as possible - slowest insert/delete in table
// -----------------------------------------------------------------
return n;
#else
// decent compromise when SPP_ALLOC_SZ == 2
// ----------------------------------------
static size_type sz_minus_1 = SPP_ALLOC_SZ - 1;
return (n + sz_minus_1) & ~sz_minus_1;
#endif
}
As s_alloc_batch_sz is static, and the code logic show that s_alloc_batch_sz will be init only once by 'if (!s_alloc_batch_sz[0])' block, duplicate calls has no effect.
So init block should be wrapped by 'std::call_once' or something like that.
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 05 Nov 2019 10:03:08 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14555/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14555/3//COMMIT_MSG@15
PS3, Line 15: acquir
> acquire
Done
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@1861
PS1, Line 1861: static const uint64_t kBlockMapMask = kBlockMapChunk - 1;
> BTW, the cache implementation (util/cache.cc) sets the number of shards to
It's a good advice.
But in fact, one of the 2 machines I ran benchmark on has 32 CPU cores, the other has 24 CPU cores.
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 02 Nov 2019 11:53:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14555
to look at the new patch set (#2).
Change subject: KUDU-2977 sharding block map to speed up tserver startup
......................................................................
KUDU-2977 sharding block map to speed up tserver startup
Separate LogBlockManager's block lock and container lock,
and sharding block map.
Time cost of LogBlockManagerTest.StartupBenchmark in old version:
I1026 17:17:30.231856 150030 log_block_manager-test.cc:1042] Time spent reopening block manager: real 1.148s user 0.092s sys 0.030s
I1026 17:17:30.798593 160153 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.062s user 0.062s sys 0.000s
I1026 17:17:30.866621 160151 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.109s user 0.068s sys 0.000s
I1026 17:17:30.949687 160154 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.192s user 0.083s sys 0.000s
I1026 17:17:31.007802 160147 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.264s user 0.058s sys 0.000s
I1026 17:17:31.103804 160149 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.339s user 0.094s sys 0.002s
I1026 17:17:31.171495 160150 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.417s user 0.068s sys 0.000s
I1026 17:17:31.252955 160148 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.502s user 0.081s sys 0.000s
I1026 17:17:31.331007 160152 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.581s user 0.078s sys 0.000s
new version:
I1026 17:15:48.425242 52360 log_block_manager-test.cc:1042] Time spent reopening block manager: real 0.945s user 0.159s sys 0.001s
I1026 17:15:49.245585 64003 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.256s user 0.162s sys 0.022s
I1026 17:15:49.323187 63997 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.315s user 0.190s sys 0.027s
I1026 17:15:49.351920 64004 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.330s user 0.213s sys 0.042s
I1026 17:15:49.377976 63999 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.341s user 0.229s sys 0.032s
I1026 17:15:49.380051 64002 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.336s user 0.208s sys 0.034s
I1026 17:15:49.387194 63998 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.330s user 0.215s sys 0.040s
I1026 17:15:49.399719 64001 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.317s user 0.206s sys 0.034s
I1026 17:15:49.434571 64000 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.322s user 0.187s sys 0.061s
Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 156 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/14555/2
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14555/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14555/1//COMMIT_MSG@11
PS1, Line 11:
> Could you summarize this finding and add it to the commit message?
Done
http://gerrit.cloudera.org:8080/#/c/14555/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14555/2/src/kudu/fs/log_block_manager.h@409
PS2, Line 409: // Block IDs container used to prevent collisions when creating new anonymous blocks.
> Nit: the word "container" is already used elsewhere in the log block manage
Done
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h@410
PS1, Line 410: struct ManagedBlockShard {
> Why do we need the indirection provided by a unique_ptr? Couldn't we just d
For simple_spinlock, it's define by DISALLOW_COPY_AND_ASSIGN(simple_spinlock);
For BlockMap, we must construct it's object with BlockAllocator parameter, or we'll get an error like:
/home/laiyingchun/kudu_ap/thirdparty/installed/common/include/sparsepp/spp.h:1978:74: error: no matching function for call to ‘kudu::MemTrackerAllocator<std::pair<const kudu::BlockId, scoped_refptr<kudu::fs::internal::LogBlock> > >::MemTrackerAllocator()’
explicit sparsetable(size_type sz = 0, const allocator_type &alloc = allocator_type()) : ...
^~~~~~~~~~~~~~~~
Seems couldn't declare them directly.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@1861
PS1, Line 1861: static const uint64_t kBlockMapMask = kBlockMapChunk - 1;
> What was the significance of running on two servers? Different hardware?
No much significance. Server 1 has some other loaded processes on it, then I changed to run benchmark on server 2 (also different hardware), sorry to confusing you :(
commit message Done.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2288
PS1, Line 2288: if (first_failure.ok()) first_failure = s;
> Summarize and add to commit message?
Done
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2612
PS1, Line 2612: mem_usage += block_mem;
> Same.
Done
http://gerrit.cloudera.org:8080/#/c/14555/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14555/2/src/kudu/fs/log_block_manager.cc@1916
PS2, Line 1916: // Release all of the memory accounted by the blocks.
> When doing something on each shard, could you iterate on the number of elem
Done
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 09:41:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14555/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14555/3//COMMIT_MSG@15
PS3, Line 15: aquire
acquire
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@1861
PS1, Line 1861: static const uint64_t kBlockMapMask = kBlockMapChunk - 1;
> No much significance. Server 1 has some other loaded processes on it, then
BTW, the cache implementation (util/cache.cc) sets the number of shards to be equal to the number of CPUs on the machine. Did you look into doing that here? It's possible the number of shards you determined to be optimal is in fact optimal for the number of CPUs on the machines you tested, but may be less so on machines with more or fewer CPUs.
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 22:04:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 sharding block map to speed up tserver startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 sharding block map to speed up tserver startup
......................................................................
Patch Set 2:
(7 comments)
Thanks for the detailed perf testing.
Could you summarize your findings in the commit description? You don't need to provide all of the numbers, but just indicate, given a particular choice X, which option seemed best from the data you collected.
http://gerrit.cloudera.org:8080/#/c/14555/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14555/1//COMMIT_MSG@11
PS1, Line 11:
> Yea, I ran the benchmark multiple times, pretty stable.
Could you summarize this finding and add it to the commit message?
http://gerrit.cloudera.org:8080/#/c/14555/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14555/2/src/kudu/fs/log_block_manager.h@409
PS2, Line 409: // Block IDs container used to prevent collisions when creating new anonymous blocks.
Nit: the word "container" is already used elsewhere in the log block manager, so it's confusing to see it here too.
Perhaps we can call these shards? So the struct could be called ManagedBlockShard, and managed_blocks_ could be managed_block_shards_?
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h@410
PS1, Line 410: struct ManagedBlocks {
> unique_ptr is OK, Done.
Why do we need the indirection provided by a unique_ptr? Couldn't we just declare the members directly in the struct?
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@1861
PS1, Line 1861: static const uint64_t kBlockMapMask = kBlockMapChunk - 1;
> I also ran StartupBenchmark on 2 servers with different kBlockMapChunk valu
What was the significance of running on two servers? Different hardware?
Also, could you summarize this and add it to the commit message?
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2288
PS1, Line 2288: Status s = RemoveLogBlock(block_id, &lb);
> I did some benchmark for RemoveLogBlocks(in LogBlockManagerTest.StartupBenc
Summarize and add to commit message?
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2612
PS1, Line 2612: << " which already is alive from another container when "
> If we lock once before per ith shard's sub-map insertion, concurrency will
Same.
http://gerrit.cloudera.org:8080/#/c/14555/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14555/2/src/kudu/fs/log_block_manager.cc@1916
PS2, Line 1916: LogBlockManager::~LogBlockManager() {
When doing something on each shard, could you iterate on the number of elements in managed_blocks_? Something like:
for (const auto& mb : managed_blocks_) {
...
}
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 00:07:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 sharding block map to speed up tserver startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 sharding block map to speed up tserver startup
......................................................................
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/14555/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14555/1//COMMIT_MSG@11
PS1, Line 11:
Have you run the benchmark multiple times? Are the numbers stable? Could you see how they vary when the number of blocks is changed?
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager-test.cc@1001
PS1, Line 1001: // - only one data directory (typical servers have several)
Remove this.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager-test.cc@1017
PS1, Line 1017: true
Nit: prepend with /* force= */ so it's more clear what this argument means.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h@311
PS1, Line 311: bool AddLogBlock(LogBlockRefPtr lb);
By renaming this it becomes an overload of the existing AddLogBlock, which is confusing. Could you rename it (or the other function) so the difference between them is clearer.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h@408
PS1, Line 408:
If the size of each of these three vectors is the same, I'd replace them with a single vector holding a struct representing a "shard".
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h@410
PS1, Line 410: std::vector<std::shared_ptr<simple_spinlock>> blocks_locks_;
Why do the entries here and in blocks_by_block_id_arr_ need shared ownership?
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@1861
PS1, Line 1861: static const uint64_t kBlockMapChunk = 1 << 3;
Would be interesting to see how performance (and overhead) changes as this value changes. What about 2, 4, or 16 shards?
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2288
PS1, Line 2288: for (const auto& block_id : block_ids) {
If block_ids is large (i.e. we're deleting a large tablet), this will result in a lot of lock activity. Would it be better to acquire all shards' locks up front?
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2609
PS1, Line 2609: LOG_TIMING(INFO, "AddLogBlock...") {
This isn't super helpful as what we really care about is total startup time, not so much the time spent starting one data directory, right?
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2612
PS1, Line 2612: if (!AddLogBlock(std::move(e.second))) {
Likewise this can result in a lot of lock activity due to the number of entries in live_blocks.
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 28 Oct 2019 04:33:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 12 Nov 2019 08:07:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
Patch Set 5:
> Patch Set 5:
>
> > Patch Set 5:
> >
> > > Patch Set 5:
> > >
> > > The TSAN failure looks real but I don't know what's causing it.
> >
> >
> > Look more about sparsepp project https://github.com/greg7mdp/sparsepp, I found this data race issuse has been fixed, by commit:
> > commit e6aad301c37a69c7b91e0f437ee525c0a72062d4
> > Author: Breno Rodrigues Guimaraes <br...@gmail.com>
> > Date: Sun Oct 22 07:35:33 2017 -0700
> >
> > Avoid data race on initialization of s_alloc_batch_sz
> >
> > I think we should update this thirdparty library.
>
> Good find. I agree; we should probably update to the latest commit.
>
> Alternatively, the author of sparspp now recommends https://github.com/greg7mdp/parallel-hashmap, so we could switch to that instead, though we should probably profile it first.
OK, we'll do that some time later.
And sparspp upgrade patch here https://gerrit.cloudera.org/c/14643, please take a review.
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 09:24:09 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2977 sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 sharding block map to speed up tserver startup
......................................................................
Patch Set 2:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/14555/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14555/1//COMMIT_MSG@11
PS1, Line 11:
> Have you run the benchmark multiple times? Are the numbers stable? Could yo
Yea, I ran the benchmark multiple times, pretty stable.
I ran the StartupBenchmark test on 2 server, and compare old version and new version, 'real time' cost logged by LOG_TIMING like follows:
block count old new
100,000 0.0881 0.0938
1,000,000 1.177 1.0485
2,000,000 2.5675 2.1233
4,000,000 6.0655 4.6847
10,000,000 16.9558 13.9073
block count old new
100,000 0.0939 0.1006
1,000,000 1.3432 1.222
2,000,000 2.9359 2.6752
4,000,000 6.8357 5.3497
10,000,000 19.8975 14.8417
When block count increasing, startup has better performance.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager-test.cc@1001
PS1, Line 1001: // - minimal number of containers, each of which is entirely full
> Remove this.
Done
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager-test.cc@1017
PS1, Line 1017:
> Nit: prepend with /* force= */ so it's more clear what this argument means.
Done
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h@311
PS1, Line 311: bool AddLogBlock(LogBlockRefPtr lb);
> By renaming this it becomes an overload of the existing AddLogBlock, which
Done
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h@408
PS1, Line 408:
> If the size of each of these three vectors is the same, I'd replace them wi
Done
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.h@410
PS1, Line 410: struct ManagedBlocks {
> Why do the entries here and in blocks_by_block_id_arr_ need shared ownershi
unique_ptr is OK, Done.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@1861
PS1, Line 1861: static const uint64_t kBlockMapMask = kBlockMapChunk - 1;
> Would be interesting to see how performance (and overhead) changes as this
I also ran StartupBenchmark on 2 servers with different kBlockMapChunk values, time cost like follows:
sharding shift, server1, server2
1 5.383 6.2373
2 5.1782 5.5581
3 4.9536 5.4049
4 4.2259 5.3289
5 4.8093 5.5196
6 4.983 5.5974
7 5.5863 6.0348
So I think 4 shift bits (16 shards) is best, and I changed it to '1 << 4'.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2288
PS1, Line 2288: Status s = RemoveLogBlock(block_id, &lb);
> If block_ids is large (i.e. we're deleting a large tablet), this will resul
I did some benchmark for RemoveLogBlocks(in LogBlockManagerTest.StartupBenchmark add some code to remove a batch of block in a loop), the difference is very little.
Test result like follows (time cost per batch, in seconds):
batch size, old version, lock once, lock N times
1,000 0.00384425 0.00398775 0.0040495
5,000 0.0188475 0.0197075 0.01955875
10,000 0.037975 0.039765 0.0388825
20,000 0.076725 0.083855 0.08087
40,000 0.15567 0.16672 0.16199
100,000 0.407225 0.414 0.395775
* lock once:
for (int i = 0; i < kBlockMapChunk; ++i) {
std::lock_guard<simple_spinlock> l(*managed_blocks_[i].lock);
for (const auto& block_id : block_ids) {
int index = block_id.id() & kBlockMapMask;
if (index != i) continue;
LogBlockRefPtr lb;
Status s = RemoveLogBlock(i, block_id, &lb);
...
}
}
* lock N times: current code
And also there may be the same case as what I mentioned in https://gerrit.cloudera.org/c/14555/1/src/kudu/fs/log_block_manager.cc#2609, it's a common case that we remove several tablets once, e.g. remove a range partiton which has many hash partitions, or drop a table. If we hold a lock and do something last a long time , the later arrive threads will wait 'in a queue' to aquire the lock, so I think keep the locking time shorter would be better.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2609
PS1, Line 2609: if (!AddLogBlock(std::move(e.second))) {
> This isn't super helpful as what we really care about is total startup time
I use it to compare with old version. When I ran LogBlockManagerTest.StartupBenchmark with 10,000,000 blocks, old version logged like:
I1028 16:46:50.621170 4339 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 1.288s user 1.113s sys 0.175s
I1028 16:46:51.779841 4346 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 2.317s user 1.018s sys 0.140s
I1028 16:46:52.913904 4340 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 3.426s user 0.942s sys 0.187s
I1028 16:46:54.350420 4342 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 4.832s user 1.266s sys 0.171s
I1028 16:46:55.632331 4338 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 6.167s user 1.026s sys 0.253s
I1028 16:46:56.918051 4343 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 7.442s user 1.043s sys 0.244s
I1028 16:46:58.794180 4345 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 9.376s user 1.659s sys 0.218s
I1028 16:47:00.056476 4344 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 10.535s user 1.045s sys 0.216s
new version logged like:
I1028 16:42:13.491647 95613 log_block_manager.cc:2609] Time spent AddLogBlock...: real 4.268s user 2.292s sys 0.574s
I1028 16:42:13.502215 95615 log_block_manager.cc:2609] Time spent AddLogBlock...: real 4.321s user 2.078s sys 0.532s
I1028 16:42:13.609458 95611 log_block_manager.cc:2609] Time spent AddLogBlock...: real 4.256s user 2.112s sys 0.540s
I1028 16:42:13.649394 95612 log_block_manager.cc:2609] Time spent AddLogBlock...: real 4.295s user 2.046s sys 0.579s
I1028 16:42:13.655992 95614 log_block_manager.cc:2609] Time spent AddLogBlock...: real 4.322s user 2.333s sys 0.520s
I1028 16:42:13.659255 95616 log_block_manager.cc:2609] Time spent AddLogBlock...: real 4.476s user 2.228s sys 0.556s
I1028 16:42:13.674238 95617 log_block_manager.cc:2609] Time spent AddLogBlock...: real 4.208s user 2.206s sys 0.520s
I1028 16:42:13.685796 95610 log_block_manager.cc:2609] Time spent AddLogBlock...: real 4.326s user 2.157s sys 0.474s
That said new method has better concurrency.
I will remove it after test.
http://gerrit.cloudera.org:8080/#/c/14555/1/src/kudu/fs/log_block_manager.cc@2612
PS1, Line 2612: << " which already is alive from another container when "
> Likewise this can result in a lot of lock activity due to the number of ent
If we lock once before per ith shard's sub-map insertion, concurrency will be influenced as what I mentioned in https://gerrit.cloudera.org/c/14555/1/src/kudu/fs/log_block_manager.cc#2609. Test result like follows:
block count, current, lock once per sub
100,000 0.0938 0.0939
1,000,000 1.0485 1.1477
2,000,000 2.1233 2.5293
4,000,000 4.6847 5.447
10,000,000 13.9073 15.8155
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 31 Oct 2019 08:12:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14555
to look at the new patch set (#5).
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
KUDU-2977 Sharding block map to speed up tserver startup
Separate LogBlockManager's block lock and container lock,
and sharding block map.
Tablet server with multiple data directories cost much time to
startup, there is a thread for each data directory, each of them
should acquire a global lock to add blocks found in its own data
directory to the global block map, so there is a queue to acquire
lock that may cost much time. The same when remove a batch of blocks.
After bunch of benchmarks, we found pretty stable improvment of this
patch, time saving much more when block count increasing (reduce about
20% startup time when 10,000,000 blocks on 8 data directories).
Sharding count is 8, which has the best performance after comparing
with 1, 2, 4, 8, 16, 32, 64 and 128.
Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 153 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/14555/5
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
Patch Set 5:
> Patch Set 5:
>
> The TSAN failure looks real but I don't know what's causing it.
Look more about sparsepp project https://github.com/greg7mdp/sparsepp, I found this data race issuse has been fixed, by commit:
commit e6aad301c37a69c7b91e0f437ee525c0a72062d4
Author: Breno Rodrigues Guimaraes <br...@gmail.com>
Date: Sun Oct 22 07:35:33 2017 -0700
Avoid data race on initialization of s_alloc_batch_sz
I think we should update this thirdparty library.
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 05 Nov 2019 10:15:21 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
Patch Set 5:
> Patch Set 5:
>
> > Patch Set 5:
> >
> > The TSAN failure looks real but I don't know what's causing it.
>
>
> Look more about sparsepp project https://github.com/greg7mdp/sparsepp, I found this data race issuse has been fixed, by commit:
> commit e6aad301c37a69c7b91e0f437ee525c0a72062d4
> Author: Breno Rodrigues Guimaraes <br...@gmail.com>
> Date: Sun Oct 22 07:35:33 2017 -0700
>
> Avoid data race on initialization of s_alloc_batch_sz
>
> I think we should update this thirdparty library.
Good find. I agree; we should probably update to the latest commit.
Alternatively, the author of sparspp now recommends https://github.com/greg7mdp/parallel-hashmap, so we could switch to that instead, though we should probably profile it first.
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 08:09:27 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
Patch Set 5:
The TSAN failure looks real but I don't know what's causing it.
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sun, 03 Nov 2019 21:27:19 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14555 )
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
KUDU-2977 Sharding block map to speed up tserver startup
Separate LogBlockManager's block lock and container lock,
and sharding block map.
Tablet server with multiple data directories cost much time to
startup, there is a thread for each data directory, each of them
should acquire a global lock to add blocks found in its own data
directory to the global block map, so there is a queue to acquire
lock that may cost much time. The same when remove a batch of blocks.
After bunch of benchmarks, we found pretty stable improvment of this
patch, time saving much more when block count increasing (reduce about
20% startup time when 10,000,000 blocks on 8 data directories).
Sharding count is 8, which has the best performance after comparing
with 1, 2, 4, 8, 16, 32, 64 and 128.
Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Reviewed-on: http://gerrit.cloudera.org:8080/14555
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 153 insertions(+), 103 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14555
to look at the new patch set (#4).
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
KUDU-2977 Sharding block map to speed up tserver startup
Separate LogBlockManager's block lock and container lock,
and sharding block map.
Tablet server with multiple data directories cost much time to
startup, there is a thread for each data directory, each of them
should acquire a global lock to add blocks found in its own data
directory to the global block map, so there is a queue to acquire
lock that may cost much time. The same when remove a batch of blocks.
After bunch of benchmarks, we found pretty stable improvment of this
patch, time saving much more when block count increasing (reduce about
20% startup time when 10,000,000 blocks on 8 data directories).
Sharding count is 8, which has the best performance after comparing
with 1, 2, 4, 8, 16, 32, 64 and 128.
Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 153 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/14555/4
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14555
to look at the new patch set (#3).
Change subject: KUDU-2977 Sharding block map to speed up tserver startup
......................................................................
KUDU-2977 Sharding block map to speed up tserver startup
Separate LogBlockManager's block lock and container lock,
and sharding block map.
Tablet server with multiple data directories cost much time to
startup, there is a thread for each data directory, each of them
should aquire a global lock to add blocks found in its own data
directory to the global block map, so there is a queue to aquire
lock that may cost much time. The same when remove a batch of blocks.
After bunch of benchmarks, we found pretty stable improvment of this
patch, time saving much more when block count increasing (reduce about
20% startup time when 10,000,000 blocks on 8 data directories).
Sharding count is 8, which has the best performance after comparing
with 1, 2, 4, 8, 16, 32, 64 and 128.
Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 153 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/14555/3
--
To view, visit http://gerrit.cloudera.org:8080/14555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0d5c13e051a2c1d6cfd1c9ad7db8a3cd195459d
Gerrit-Change-Number: 14555
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>