[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Tue, 12 Nov 2019 08:07:51 + Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 > > 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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Wed, 06 Nov 2019 09:24:09 + Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 > 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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Wed, 06 Nov 2019 08:09:27 + Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Tue, 05 Nov 2019 10:15:21 + Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 2)
group_start_alloc /= 2;
alloc_sz += group_start_alloc;
}
}
return n ? static_cast(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 <405403...@qq.com>
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
Gerrit-Comment-Date: Tue, 05 Nov 2019 10:03:08 +
Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Sun, 03 Nov 2019 21:27:19 + Gerrit-HasComments: No
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Sat, 02 Nov 2019 11:53:43 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Fri, 01 Nov 2019 22:04:20 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 > >::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 <405403...@qq.com>
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 09:41:09 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 Sharding block map to speed up tserver startup
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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2977 sharding block map to speed up tserver startup
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 <405403...@qq.com>
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 00:07:19 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2977 sharding block map to speed up tserver startup
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.003844250.00398775 0.0040495
5,000 0.0188475 0.0197075 0.01955875
10,000 0.037975 0.0397650.0388825
20,000 0.076725 0.0838550.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 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
[kudu-CR] KUDU-2977 sharding block map to speed up tserver startup
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.148suser 0.092ssys 0.030s I1026 17:17:30.798593 160153 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.062suser 0.062ssys 0.000s I1026 17:17:30.866621 160151 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.109suser 0.068ssys 0.000s I1026 17:17:30.949687 160154 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.192suser 0.083ssys 0.000s I1026 17:17:31.007802 160147 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.264suser 0.058ssys 0.000s I1026 17:17:31.103804 160149 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.339suser 0.094ssys 0.002s I1026 17:17:31.171495 160150 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.417suser 0.068ssys 0.000s I1026 17:17:31.252955 160148 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.502suser 0.081ssys 0.000s I1026 17:17:31.331007 160152 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.581suser 0.078ssys 0.000s new version: I1026 17:15:48.425242 52360 log_block_manager-test.cc:1042] Time spent reopening block manager: real 0.945suser 0.159ssys 0.001s I1026 17:15:49.245585 64003 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.256suser 0.162ssys 0.022s I1026 17:15:49.323187 63997 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.315suser 0.190ssys 0.027s I1026 17:15:49.351920 64004 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.330suser 0.213ssys 0.042s I1026 17:15:49.377976 63999 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.341suser 0.229ssys 0.032s I1026 17:15:49.380051 64002 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.336suser 0.208ssys 0.034s I1026 17:15:49.387194 63998 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.330suser 0.215ssys 0.040s I1026 17:15:49.399719 64001 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.317suser 0.206ssys 0.034s I1026 17:15:49.434571 64000 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.322suser 0.187ssys 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 <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2977 sharding block map to speed up tserver startup
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> 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 <405403...@qq.com>
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 28 Oct 2019 04:33:23 +
Gerrit-HasComments: Yes
[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.148suser 0.092ssys 0.030s I1026 17:17:30.798593 160153 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.062suser 0.062ssys 0.000s I1026 17:17:30.866621 160151 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.109suser 0.068ssys 0.000s I1026 17:17:30.949687 160154 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.192suser 0.083ssys 0.000s I1026 17:17:31.007802 160147 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.264suser 0.058ssys 0.000s I1026 17:17:31.103804 160149 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.339suser 0.094ssys 0.002s I1026 17:17:31.171495 160150 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.417suser 0.068ssys 0.000s I1026 17:17:31.252955 160148 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.502suser 0.081ssys 0.000s I1026 17:17:31.331007 160152 log_block_manager.cc:2588] Time spent AddLogBlocks...: real 0.581suser 0.078ssys 0.000s new version: I1026 17:15:48.425242 52360 log_block_manager-test.cc:1042] Time spent reopening block manager: real 0.945suser 0.159ssys 0.001s I1026 17:15:49.245585 64003 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.256suser 0.162ssys 0.022s I1026 17:15:49.323187 63997 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.315suser 0.190ssys 0.027s I1026 17:15:49.351920 64004 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.330suser 0.213ssys 0.042s I1026 17:15:49.377976 63999 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.341suser 0.229ssys 0.032s I1026 17:15:49.380051 64002 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.336suser 0.208ssys 0.034s I1026 17:15:49.387194 63998 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.330suser 0.215ssys 0.040s I1026 17:15:49.399719 64001 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.317suser 0.206ssys 0.034s I1026 17:15:49.434571 64000 log_block_manager.cc:2607] Time spent AddLogBlock...: real 0.322suser 0.187ssys 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 <405403...@qq.com>
