Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/9576 )
Change subject: IMPALA-6638: Reduce file handle cache lock contention ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/9576/2/be/src/runtime/io/handle-cache.inline.h File be/src/runtime/io/handle-cache.inline.h: http://gerrit.cloudera.org:8080/#/c/9576/2/be/src/runtime/io/handle-cache.inline.h@128 PS2, Line 128: This only happens if there is no entry for this file or all the : // existing file handles are in use. > That was true while holding the lock above but is no longer necessarily tru Changed this to insert in the front and updated this comment about why. Also added a comment when iterating above to explain why iterating in order is important. http://gerrit.cloudera.org:8080/#/c/9576/2/be/src/runtime/io/handle-cache.inline.h@130 PS2, Line 130: matter whether we insert this entry before or after or in between the : // existing entries for this file > why is the previous sentence needed in order for this to be okay? This was poorly worded. What I meant is that not much time has passed since we saw that there wasn't any entry available. Any ordering decision is mostly unimportant under that circumstance. I thought about it some more, and we are very slightly better off explicitly specifying the ordering. It is mostly an edge case, but inserting at the front solves it. http://gerrit.cloudera.org:8080/#/c/9576/2/be/src/runtime/io/handle-cache.inline.h@134 PS2, Line 134: FileHandleEntry entry(new_fh, p.lru_list); : typename MapType::iterator new_it = p.cache.emplace(*fname, std::move(entry)); > I would remove the constructor and move the arguments to emplace: emplace(* I appreciate the comment, but I try to avoid style changes in adjacent code. http://gerrit.cloudera.org:8080/#/c/9576/2/be/src/runtime/io/handle-cache.inline.h@137 PS2, Line 137: ++p.size; : if (p.size > p.capacity) EvictHandles(p); > I would prefer to move this before the creation of the new entry, to make i I appreciate the comment, but I try to avoid style changes in adjacent code. http://gerrit.cloudera.org:8080/#/c/9576/2/be/src/runtime/io/handle-cache.inline.h@139 PS2, Line 139: DCHECK(!new_elem->in_use); > This DCHECK could be probably removed. This could go either way. We are relying on the constructor setting in_use to false. I figure a DCHECK doesn't hurt. -- To view, visit http://gerrit.cloudera.org:8080/9576 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c695b21ca556e9c73c703c0c891e64939271c8d Gerrit-Change-Number: 9576 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Mar 2018 21:51:19 +0000 Gerrit-HasComments: Yes
