Tim Armstrong has posted comments on this change. Change subject: IMPALA-5352: Age out unused file handles from the cache ......................................................................
Patch Set 2: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/7640/2/be/src/runtime/disk-io-mgr-handle-cache.inline.h File be/src/runtime/disk-io-mgr-handle-cache.inline.h: Line 49: shut_down_.Store(SHUTDOWN_FALSE); nit: I think we can just initialize this in the initialiser list. Line 67: if (eviction_thread_.get() != nullptr) { nit: conditional fits on one line. PS2, Line 116: // This emplace_hint requires constructing a pair in the map. Emplacing an : // element with a multiple argument constructor (such as FileHandleEntry) : // introduces ambiguity between the key's arguments and the value's arguments. : // To resolve the ambiguity, use a piecewise construction and separate the : // arguments for the key from the value. : typename MapType::iterator new_it = p.cache.emplace_hint(range.second, : std::piecewise_construct, std::forward_as_tuple(*fname), : std::forward_as_tuple(new_fh, p.lru_list)); > Is this really necessary? I'm not sure how performance sensitive this code Yeah I'm wondering if there's a simpler way to express it. I think we'll automatically use the move constructor even if we do constructor temporaries. I'm not sure if one of the following works: tuple(*fname), tuple(new_fh, p.lru_list) or maybe even: {*fname}, {new_fn, p.lru_list} Although I see that the pattern you're using appears in plenty of places on the internet so maybe there's some reason why the above doesn't work. -- To view, visit http://gerrit.cloudera.org:8080/7640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iefe04b3e2e22123ecb8b3e494934c93dfb29682e Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
