impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4623: Enable file handle cache
Date Mon, 10 Apr 2017 23:06:21 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4623: Enable file handle cache

Patch Set 3:


initial thoughts on the partitioned map.
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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe5ff60971dd653c3b6a0e13928cfa9fc59d078d
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message