impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5352: Age out unused file handles from the cache
Date Fri, 18 Aug 2017 16:55:34 GMT
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

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
             :     // 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
             :     // 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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefe04b3e2e22123ecb8b3e494934c93dfb29682e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message