impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5238: transfer reservations between trackers
Date Wed, 10 May 2017 20:58:33 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message