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/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. > I don't think I understand this. Suppose ThreadLocalCache were templatized yea, but I'd like to also extend this cache to other use cases without having to make a separate ThreadLocalCache for each type of cached object. ie we can have a constant-sized cache which includes some entries which are BloomFileReader and others which are BlockCache entries, etc. If you think that use case isn't that interesting, I could make the whole thing templatized and have N separate caches for N different types. I don't really have a strong opinion. Line 53: static constexpr int kItemCapacity = 4; > It must also be <= sizeof last_hit_. you mean < (1 << (sizeof(last_hit_) * 8)) right? Line 66: for (int i = 0; i < kItemCapacity; i++, last_hit_++) { > Would it be more performant to: actually went back and forth on this, not sure which is more performant, but it's probably not a hot spot either way. Will change to that way if you think it's more clear. PS4, Line 67: % > As per L52, didn't you intend to use bit shifting here? % with a constant power of two will turn into bit-shifting after optimization, and I think % is more clear than & (kItemCapacity - 1) Line 103: using EntryPair = std::pair<Key, std::unique_ptr<ThreadLocalCacheable>>; > Would it be possible to avoid allocations entirely? Perhaps by templatizing Same as above -- we'd have to make this class templatized on the type, vs being generic. -- 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
