Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4623: Enable file handle cache ......................................................................
Patch Set 3: (11 comments) initial thoughts on the partitioned map. http://gerrit.cloudera.org:8080/#/c/6478/3/be/src/util/lru-cache.h File be/src/util/lru-cache.h: Line 67: typedef void (*DeleterFn)(Value&); why now a ref? Line 82: /// Variant of Put with move semantics. is this needed? Line 91: bool Pop(const Key& k, Value& out); we typically don't use refs for this purpose. why the switch? Line 102: static void DummyDeleter(Value& v) {} make private Line 115: typedef std::pair<Key, Value> ValueType; value of what? Line 139: /// Wrapper around FifoMultimap that hashes the keys into a fixed number of partitions which hash fn? Line 146: template<typename Key, typename Value> you can make the number of partitions another template parameter, that way you can use a std::array for cache_partitions_. i also don't see a real need for the unique_ptr in there. fewer indirection should be faster. it would also be good to put the individual Fifomultimap objects on cache line boundaries. alignas might help here. Line 151: /// See FifoMultimap this doesn't explain how you deal with the remainder. you could also simplify this and stipulate that capacity is per-partition. http://gerrit.cloudera.org:8080/#/c/6478/3/be/src/util/lru-cache.inline.h File be/src/util/lru-cache.inline.h: Line 36: const typename MapType::value_type& val = i think if you use 'using' for MapType instead of typedef you can get rid of the typename. Line 37: std::make_pair(k, lru_list_.emplace(lru_list_.end(), k, std::move(v))); is the explicit move needed for the rvalue v? Line 51: void FifoMultimap<Key, Value>::Put(const Key& k, const Value& v) { there should be a way not to require basically a verbatim copy of this function -- To view, visit http://gerrit.cloudera.org:8080/6478 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibe5ff60971dd653c3b6a0e13928cfa9fc59d078d Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
