impala-dev 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-3201: reservation implementation for new buffer pool
Date Thu, 18 Aug 2016 00:25:53 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3201: reservation implementation for new buffer pool
......................................................................


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/3993/4/be/src/bufferpool/reservation-tracker.h
File be/src/bufferpool/reservation-tracker.h:

Line 50: /// * A tracker's reservation is at most its reservation limit.
> does 'reservation' mean 'remaining reservation' or 'what has been deducted 
Maybe I misunderstood the point. I don't think it's either of those.

Reservation means the total amount of buffer pool memory that the subtree is entitled to use.
Rather than the amount it's entitled to use minus the amount that it's used minus the amount
it's children have reserved.

I.e. using a reservation means:
  usage_ += amount;

And the invariant is:
  usage + child_reservations <= reservation <= limit

And memory consumption on the MemTracker is:
  reservation

If I understood your first alternative correctly, you mean:

Using a reservation requires:
  usage += amount;
  reservation -= amount;

And the (slightly weaker) invariant is:
  usage + child_reservations + reservation <= limit

(it's slightly weaker because it can't detect after-the-fact if usage was increased without
getting a reservation - you need a DCHECK_LE(amount, reservation) before every increment of
usage_)

Memory consumption on the MemTracker is:
  usage + child_reservations + reservation


Line 51: /// * A tracker's reservation is at least the sum of its childrens' reservations
plus
> however, that would contradict this sentence (because my children eat into 
I'm not sure I understand this comment (mabe a consequence of not understanding the first
one).

I added some more explicit notation from the above comment since it seems worthwhile anyway.


Line 59: /// as consumption because reserved memory is committed. An existing reservation
can
> we need to be careful to differentiate actual consumption vs. used-up reser
I plan to reflect usage, reservation, and limit in profile counters for diagnostic purposes
(I have had a draft follow-up patch for this hanging around for ages but didn't want to expand
the scope of this patch further).


Line 60: /// therefore always be used without increasing consumption and with no risk of exceeding
> this seems to contradict what you said earlier.
I don't think I understand. 

This point is that a call to IncreaseUsage() (e.g. from allocating a buffer) can't push a
client over a memory limit. (since the having enough unused reservation is a precondition
for calling IncreaseUsage()).


Line 70:   void InitRootTracker(MemTracker* mem_tracker, int64_t reservation);
> could the root be expressed as having parent == null?
Definitely. This approach allows stronger enforcement of invariants though. Otherwise if you
had a bug in Prepare() that didn't initialize a tracker, you could end up with a NULL parent
tracker, a broken tracker hierarchy and consequent bizarre behaviour.


Line 74:   /// children will be counted as consumption against 'mem_tracker'.
> if the reservation is really counted immediately against a memtracker (with
The MemTracker also tracks other memory that was allocated outside of the buffer pool and
has no corresponding reservation. It doesn't track how much of the reservation is used (i.e.
only deals with limit/consumption versus limit/reservation/usage). It also can't enforce limits
on spillable buffer pool memory (which we need at the query and process level).


Line 103:   /// Increase the usage. The tracker must have at least 'amount' remaining in its
> i find this terminology confusing, as explained above. do you really mean '
I don't think I understand this comment.

I don't think this was your point, but I changed this comment to use the more consistent term
of 'unused reservation'.


Line 118:   int64_t GetUsage();
> i find this term ambiguous as well, because it seems to imply the memory is
Not sure if you were just suggesting a terminological change or suggesting to track usage
externally of ReservationTracker.

Usage here does mean "usage of the reservation" not "usage of memory". Although they're equal.

I think we need to track usage internally otherwise it's difficult to force meaningful invariants.
E.g. no way to enforce reservation + usage <= limit.


-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message