Todd Lipcon has posted comments on this change. Change subject: KUDU-680. bloomfile: switch to a threadlocal cache ......................................................................
Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/6569/4/src/kudu/cfile/bloomfile.cc File src/kudu/cfile/bloomfile.cc: Line 175 > I imagine mem_consumption_ was here to handle the change in memory consumpt put this stuff back. Line 199: ReaderOptions options) > warning: parameter 'options' is unused [misc-unused-parameters] Done Line 267: // If we didn't hit in the cache, make a new cache entry and instantiate a reader. > Based on how you're using the ThreadLocalCache, I think a single LookupOrIn I'm not sure that's great, because then we have to always pass the constructor parameters into LookupOrInsertNew (I moved the IndexTreeIterator constructor parameters into BloomCacheItem to avoid an extra indirection). Plus I'm not sure I follow how it reduces a modulo, since the InsertNew function uses next_slot_ which is different than last_hit_ Line 301: bci->cur_block_pointer = std::move(bblk_ptr); > warning: passing result of std::move() as a const reference argument; no mo Done http://gerrit.cloudera.org:8080/#/c/6569/4/src/kudu/util/mt-threadlocal-test.cc File src/kudu/util/mt-threadlocal-test.cc: Line 329: TEST_F(ThreadLocalTest, TestThreadLocalCache) { > What about a multi-threaded test to verify that it's really a thread-local meh, I'm gonna skip this one, because we've already tested ThreadLocal above, and the cache's usage of threadlocals is not particularly interesting. -- To view, visit http://gerrit.cloudera.org:8080/6569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id8efd7f52eb376de2a9c445458827721806d9da8 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
