Adar Dembo has posted comments on this change. Change subject: KUDU-680. bloomfile: switch to a threadlocal cache ......................................................................
Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/6569/4/src/kudu/util/threadlocal_cache.h File src/kudu/util/threadlocal_cache.h: PS4, Line 30: // This only ensures that the ThreadLocalCache can properly destruct : // entries when the thread exits. > yea, but I'd like to also extend this cache to other use cases without havi Ah, didn't realize the goal was to share a cache like that. Do you think a shared cache would remain performant as more objects types were added? Let's take your example of a single cache used for both bloom file readers and block cache entries. Assuming there's one "hot" object for each type and a split access pattern, the best we can hope for is "ping-ponging" between the two hot objects, right? Plus, as more object types are added, kItemCapacity will need to grow, at which point there'd be no discernible difference between multiple 4-item caches and one n-item cache, right? Am I looking at this the right way? To me this suggests that a multi-cache approach would be net better. Line 53: static constexpr int kItemCapacity = 4; > you mean < (1 << (sizeof(last_hit_) * 8)) right? Yes, my bad. Line 66: for (int i = 0; i < kItemCapacity; i++, last_hit_++) { > actually went back and forth on this, not sure which is more performant, bu Yeah, I would definitely find it more clear if last_hit_ is only modified during a real hit. PS4, Line 67: % > % with a constant power of two will turn into bit-shifting after optimizati Oh ok, didn't realize you were referring to the compiler when you wrote "modulo can be implemented with bit-shifting". -- 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
