Tim Armstrong has posted comments on this change. Change subject: IMPALA-5238: transfer reservations between trackers ......................................................................
Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/6708/5/be/src/runtime/bufferpool/reservation-tracker.cc File be/src/runtime/bufferpool/reservation-tracker.cc: Line 237: DCHECK_EQ(path_to_root.back(), other_path_to_root.back()); > maybe first dcheck they both aren't empty to support calling back() Seems like overkill to me since FindPathToRoot() always includes the current tracker by definition. I can add it if you feel strongly. PS5, Line 253: > nit: indentation mismatch Done Line 274: if (i == other_path_to_root.size() - 1 && curr->parent_ != nullptr) { > this path could use a quick explanation We actually don't need it - it was only needed to offset an increment in DecreaseReservationInternal(). I was able to remove this and the increment. Line 278: } > why is it okay to add the reservation into the other chain before subtracti There is a race if you do that but the bug would be that the caller was misusing the API and creating a semantic race. E.g. if two threads call IncreaseReservationToFit() and TransferReservationTo() concurrently, and TransferReservationTo() runs second, the post-condition of IncreaseReservationToFit() can become false. Or if AllocateFrom(x) and TransferReservationTo(y) are called concurrently and the current reservation is < x - y, then the one that runs second will DCHECK. It's safe in that a concurrent thread can't get memory that it wouldn't be entitled to, because we did the increase before the decrease. A concurrent thread could probably notice an anomaly if it was looking for one, but I can't think of any scenario where that is actually a problem in practice. It is tricky to reason about - I tried augmenting the lock ordering so that we can lock everything at the same time and therefore fully guarantee atomicity. Seemed to work out ok so going with that now. Line 281: DecreaseReservation(bytes, false, path_to_root.back()); > is this always safe while holding 'locks'? Why can't we get into a deadloc Yeah that was violating the lock order - I removed the scoped block without thinking about that. http://gerrit.cloudera.org:8080/#/c/6708/5/be/src/runtime/bufferpool/reservation-tracker.h File be/src/runtime/bufferpool/reservation-tracker.h: PS5, Line 194: Decrase > typo Done -- To view, visit http://gerrit.cloudera.org:8080/6708 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I21f008abaf1aa4fcd2d854769a603b97589af3b3 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
