impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
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:

(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 <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message