subject:"\[Impala\-ASF\-CR\] IMPALA\-3201\: reservation implementation for new buffer pool"

Tim Armstrong has posted comments on this change.

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


Patch Set 16: Code-Review+2

The added DCHECK caused a failure in another backend test where the MemTrackers 
are set up in an unexpected way - will remove the DCHECK temporarily and fix in 
a follow-up change.

-- 
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: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Hello Internal Jenkins, Dan Hecht,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3993

to look at the new patch set (#16).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool. The reservations are always guaranteed and any buffer/page
allocations require a reservation. Reservations are tracked via a
hierarchy of ReservationTrackers.

Eventually ReservationTrackers should become the main mechanism for
memory accounting, once all runtime memory is handled by the buffer
pool. In the meantime, query/process limits are enforced and memory
is reported through the preexisting MemTracker hierarchy.

Reservations are accounted as consumption against a MemTracker because
the memory is committed and cannot be used for other purposes. The
MemTracker then uses the counters from the ReservationTracker to log
information about buffer-pool memory down to the operator level.

Testing:
Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker-counters.h
A be/src/bufferpool/reservation-tracker-test.cc
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
A be/src/util/dummy-runtime-profile.h
M be/src/util/runtime-profile-counters.h
13 files changed, 1,439 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/16
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-09-08 Thread Internal Jenkins (Code Review)

Internal Jenkins has posted comments on this change.

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


Patch Set 15: Verified-1

Build failed: 
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/177/

-- 
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has posted comments on this change.

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


Patch Set 13:

(1 comment)

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

Line 105:   /// if a system error occurs.
> This sentence isn't right.
Done


-- 
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Hello Dan Hecht,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3993

to look at the new patch set (#14).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool. The reservations are always guaranteed and any buffer/page
allocations require a reservation. Reservations are tracked via a
hierarchy of ReservationTrackers.

Eventually ReservationTrackers should become the main mechanism for
memory accounting, once all runtime memory is handled by the buffer
pool. In the meantime, query/process limits are enforced and memory
is reported through the preexisting MemTracker hierarchy.

Reservations are accounted as consumption against a MemTracker because
the memory is committed and cannot be used for other purposes. The
MemTracker then uses the counters from the ReservationTracker to log
information about buffer-pool memory down to the operator level.

Testing:
Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker-counters.h
A be/src/bufferpool/reservation-tracker-test.cc
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
A be/src/util/dummy-runtime-profile.h
M be/src/util/runtime-profile-counters.h
13 files changed, 1,440 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/14
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-09-08 Thread Dan Hecht (Code Review)

Dan Hecht has posted comments on this change.

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


Patch Set 13: Code-Review+2

(1 comment)

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

Line 105:   /// if a system error occurs.
This sentence isn't right.


-- 
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has posted comments on this change.

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


Patch Set 10:

(3 comments)

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

PS10, Line 100: MemLimitExceeded
> Having the caller directly use IncreaseReservation() to acquire its initial
Done


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

Line 178:   int64_t bytes, bool use_existing_reservation, bool 
is_child_reservation);
> yeah, didn't suggest getting rid of the unlocked one cause now the child ac
Yeah it doesn't seem ideal either way. I think this reduces the degree of 
indirection a bit so may be easier to trace.


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

Line 120:   /// made to the state of any ReservationTrackers.
> formatting glitch
Done


-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has uploaded a new patch set (#13).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool. The reservations are always guaranteed and any buffer/page
allocations require a reservation. Reservations are tracked via a
hierarchy of ReservationTrackers.

Eventually ReservationTrackers should become the main mechanism for
memory accounting, once all runtime memory is handled by the buffer
pool. In the meantime, query/process limits are enforced and memory
is reported through the preexisting MemTracker hierarchy.

Reservations are accounted as consumption against a MemTracker because
the memory is committed and cannot be used for other purposes. The
MemTracker then uses the counters from the ReservationTracker to log
information about buffer-pool memory down to the operator level.

Testing:
Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker-counters.h
A be/src/bufferpool/reservation-tracker-test.cc
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
A be/src/util/dummy-runtime-profile.h
M be/src/util/runtime-profile-counters.h
13 files changed, 1,442 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/13
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-09-07 Thread Dan Hecht (Code Review)

Dan Hecht has posted comments on this change.

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


Patch Set 10:

(4 comments)

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

PS10, Line 100: MemLimitExceeded
> I was deferring it until I can solve it more holistically in the context of
Having the caller directly use IncreaseReservation() to acquire its initial 
reservation is good with me.  Fragment-instances/exec-nodes still have a 
concept of initial reservation but I agree, no need to carry it all the way 
down to this level in the code.


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

Line 121:   bool IncreaseReservation(int64_t bytes);
> It will be useful in setting up initial reservations: we can request the en
Adding the TODO and revisiting later is okay with me.


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

Line 178:   int64_t bytes, bool use_existing_reservation, bool 
is_child_reservation);
> Removed IncreaseReservationInternal() by just having all callsites acquire 
yeah, didn't suggest getting rid of the unlocked one cause now the child 
acquires the parent lock which seems a little messy.  But I don't feel too 
strongly about that.


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

Line 120:   /// made to the state of any ReservationTrackers.
formatting glitch


-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has posted comments on this change.

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


Patch Set 11:

(1 comment)

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

Line 101: status.AddDetail("Could not get initial reservation");
> should this include 'initial_reservation' and maybe some label to make it e
Will wait until we decide whether to just remove initial_reservation. We could 
remove the Status return in that case too.


-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has uploaded a new patch set (#12).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool. The reservations are always guaranteed and any buffer/page
allocations require a reservation. Reservations are tracked via a
hierarchy of ReservationTrackers.

Eventually ReservationTrackers should become the main mechanism for
memory accounting, once all runtime memory is handled by the buffer
pool. In the meantime, query/process limits are enforced and memory
is reported through the preexisting MemTracker hierarchy.

Reservations are accounted as consumption against a MemTracker because
the memory is committed and cannot be used for other purposes. The
MemTracker then uses the counters from the ReservationTracker to log
information about buffer-pool memory down to the operator level.

Testing:
Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker-counters.h
A be/src/bufferpool/reservation-tracker-test.cc
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
A be/src/util/dummy-runtime-profile.h
M be/src/util/runtime-profile-counters.h
13 files changed, 1,462 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/12
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has posted comments on this change.

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


Patch Set 9:

(10 comments)

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

PS10, Line 100: 
> Shouldn't we introduce a Reservation-specific error code?  Okay to defer fo
I was deferring it until I can solve it more holistically in the context of 
query startup. E.g. if we fail to get the initial reservation I think we want 
an error message that reports the full reservation it was trying to acquire 
across all exec nodes.

Now that I think about it, maybe I should just remove the initial_reservation 
argument/concept from the ReservationTracker. Not reason it can't be 
implemented with InitChildTracker() then IncreaseReservation(). What do you 
think?


Line 143: 
> This comment is confusing because it sounds like this is a precondition to 
Done


Line 145:   return IncreaseReservationInternal(bytes, false, false);
> the method comment claims that this unlinks from parent, but parent_ is not
Done


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

Line 121:   bool IncreaseReservation(int64_t bytes);
> My motivation for bringing it up is because it'd be nice to remove the !use
It will be useful in setting up initial reservations: we can request the entire 
initial reservation at the query level, then during Prepare() each exec node 
can pull down its portion of the reservation.

As you mentioned it's also useful (necessary, even) if an exec node wants to 
subdivide its initial reservation. With the current primitives you can't 
release the reservation to the next level - that is maybe something that we 
will want to add.

During query execution we can also implement different policies about how much 
reservation each query should hold onto. It's not clear to me yet what the 
right policy will be, particularly once we need to transfer reservation between 
different non-overlapping fragments.

My feeling is that it makes sense to think about those policies more 
holistically in a later step when we can experiment with real queries and 
implement any ReservationTracker changes/extensions then (I don't see any 
obstacles to changing the policy with the current implementation).

I added a TODO to DecreaseReservation() to decide on this and documented the 
current policy. I documented IncreaseReservation() and 
IncreaseReservationToFit().


Line 231:   MemTracker* parent_mem_tracker_;
> Or we could have ParentMemTracker() { return parent_ == NULL ? NULL: parent
Done


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

Line 114:   /// before calling Close().
> is there a requirement that this tracker's children must have been closed b
This is actually already DCHECKed by calling DecreaseReservationInternal() on 
the parent when closing.
It should definitely be one since otherwise it's not clear what the expected 
behaviour of calling any method aside from Close() on the child is.

Updated the comment.


PS10, Line 130: '.
> this one was missed: ... before calling this method.
Done


Line 178: 
> this would be easier to follow if we needed need both the unlocked and lock
Removed IncreaseReservationInternal() by just having all callsites acquire the 
lock.


PS10, Line 180: ns tr
> false
Done


Line 194:   void CheckConsistency() const;
> In debug builds ...
Done


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has uploaded a new patch set (#11).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool. The reservations are always guaranteed and any buffer/page
allocations require a reservation. Reservations are tracked via a
hierarchy of ReservationTrackers.

Eventually ReservationTrackers should become the main mechanism for
memory accounting, once all runtime memory is handled by the buffer
pool. In the meantime, query/process limits are enforced and memory
is reported through the preexisting MemTracker hierarchy.

Reservations are accounted as consumption against a MemTracker because
the memory is committed and cannot be used for other purposes. The
MemTracker then uses the counters from the ReservationTracker to log
information about buffer-pool memory down to the operator level.

Testing:
Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker-counters.h
A be/src/bufferpool/reservation-tracker-test.cc
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
A be/src/util/dummy-runtime-profile.h
M be/src/util/runtime-profile-counters.h
13 files changed, 1,462 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/11
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-09-07 Thread Dan Hecht (Code Review)

Dan Hecht has posted comments on this change.

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


Patch Set 10:

(12 comments)

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

PS10, Line 100: MemLimitExceeded
Shouldn't we introduce a Reservation-specific error code?  Okay to defer for 
now but wondering what the plan is.


Line 101: status.AddDetail("Could not get initial reservation");
should this include 'initial_reservation' and maybe some label to make it 
easier to debug?


Line 143:   // the reservation.
This comment is confusing because it sounds like this is a precondition to 
calling Close(), but then line 144 does what it says has to be done before 
closing.  Reword.


Line 145:   mem_tracker_ = NULL;
the method comment claims that this unlinks from parent, but parent_ is not 
reset.  Should it?


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

Line 121:   bool IncreaseReservation(int64_t bytes);
> IncreaseReservation() seems like the better primitive since you can impleme
My motivation for bringing it up is because it'd be nice to remove the 
!use_existing_reservation case, which would simplify things.  So making this 
private doesn't help.

Also, related, it seems a bit weird that IncreaseReservation() will use a 
parent's pre-existing reservation but then DecreaseReservation() will force the 
parent to relinquish the reservation.  Why is that the right thing?  And given 
that Decrease always forces the parent to give back reservation, when would 
Increase even have the opportunity to use the parent's (already acquired) 
reservation? I guess the expectation is that some clients will get a 
reservation at, say, the exec-node level, and then later use that reservation 
at a child to the exec-node?  But then, again, how does that fit in with the 
Decrease behavior?

In any case, this difference is subtle and needs to be called out in the 
Increase and Decrease methods (I.e. they don't explain how the hierarchy comes 
into play).

Alternatively, I wonder if instead of DecreaseReservation() we should just have 
a ClearReservation(), which seems to have clearer semantics w.r.t. the parent 
reservation.  But I'm first looking to understand how DecreaseReservation() 
will be used.


Line 231: 
> Removed it. The checks get a little more complicated (have to check if both
Or we could have ParentMemTracker() { return parent_ == NULL ? NULL: 
parent_->mem_tracker_; }

but I'm fine with the new code too.


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

Line 114:   /// before calling Close().
is there a requirement that this tracker's children must have been closed 
before calling Close() on it?


PS10, Line 130: '.
this one was missed: ... before calling this method.


Line 178:   int64_t bytes, bool use_existing_reservation, bool 
is_child_reservation);
this would be easier to follow if we needed need both the unlocked and locked 
variants, but it's not immediately obvious how to get rid of the locked given 
the call paths from Init (and Close for Decrease).  But maybe you can think of 
a clean way.


PS10, Line 180: error
false


Line 194:   /// Check the internal consistency of the ReservationTracker.
In debug builds ...

or mention that this results in DCHECKs.  Might be even clearer to rename it 
DCheckConsistency(). Or DCheckInvariants().


http://gerrit.cloudera.org:8080/#/c/3993/10/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

PS10, Line 208: 
thanks for fixing this, it always confused me.


-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-09-02 Thread Tim Armstrong (Code Review)

Tim Armstrong has posted comments on this change.

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


Patch Set 9:

(13 comments)

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

Line 33:   /// The tracker's current reservation in bytes.
As discussed, I removed the "current" counters since it's not particularly 
useful to report in profiles.


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

Line 121:   bool IncreaseReservation(int64_t bytes);
> Still not clear to me if we need both variants.  But we can leave for now a
IncreaseReservation() seems like the better primitive since you can implement 
everything in terms of it. It's convenient for tests too. We could potentially 
make it a private method for testing only.

IncreaseReservationToFit() is relatively easy to implement on top of 
IncreaseReservation() but has a more complicated contract, I just added it to 
the interface originally to avoid duplicating the calculation all over the 
place. I thought about making it an external utility method to compute the 
required reservation increase, but that got verbose.


PS9, Line 130: '.
> before calling this method (i.e. clarify it's not a global invariant or pos
Done


PS9, Line 134: .
> before calling this method.
Done


PS9, Line 137: resources
> memory (now that we're being a bit less abstract)
Done


PS9, Line 169: true
> what does this do if it's false?
Done


PS9, Line 179: Return
> For the topmost link, return false if ...
Done


Line 196:   /// Increase or decrease 'used_reservation_' and update counters 
accordingly.
> profile counters
Done


PS9, Line 200: counters
> profile counters
Done


PS9, Line 217: If no RuntimeProfile was provided, all counters are NULL.
> update
Done


Line 231:   MemTracker* parent_mem_tracker_;
> with this simplification, is this worth it, or should we just do parent_->m
Removed it. The checks get a little more complicated (have to check if both 
parent and parent->mem_tracker are NULL) but it works out ok.


http://gerrit.cloudera.org:8080/#/c/3993/9/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

Line 499: };
> why is this header the right place (as opposed to runtime-profile.h)?
I didn't want to put in in runtime-profile.h since it would be polluting a 
widely-included header with a rarely used class.

I moved to dummy-runtime-profile.h


http://gerrit.cloudera.org:8080/#/c/3993/9/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

Line 27: #include "common/object-pool.h"
> why? i only see indirect references to ObjectPool.
Needed for DummyProfile - missed moving it to the other file. Now in 
dummy-runtime-profile.h


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-09-02 Thread Tim Armstrong (Code Review)

Tim Armstrong has uploaded a new patch set (#10).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool. The reservations are always guaranteed and any buffer/page
allocations require a reservation. Reservations are tracked via a
hierarchy of ReservationTrackers.

Eventually ReservationTrackers should become the main mechanism for
memory accounting, once all runtime memory is handled by the buffer
pool. In the meantime, query/process limits are enforced and memory
is reported through the preexisting MemTracker hierarchy.

Reservations are accounted as consumption against a MemTracker because
the memory is committed and cannot be used for other purposes. The
MemTracker then uses the counters from the ReservationTracker to log
information about buffer-pool memory down to the operator level.

Testing:
Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker-counters.h
A be/src/bufferpool/reservation-tracker-test.cc
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
A be/src/util/dummy-runtime-profile.h
M be/src/util/runtime-profile-counters.h
13 files changed, 1,454 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/10
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-09-01 Thread Dan Hecht (Code Review)

Dan Hecht has posted comments on this change.

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

Patch Set 8:

(16 comments)

Only partially made it through the rest of the change, still going. But wanted
to flush out my response to the reservation-tracker-counters.h thread.

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

PS8, Line 35: Counter
> There's an UpdateReservation() method I added in the last patchset that doe
Right it's similar to UpdateReservation(), but encapsulated in this class so
that ReservatIonTracker() doesn't have to know about this oddity of the profile
counters.

Also, I was envisioning then that this class would expose accessors:
reservation(), used_reservation(), reservation_limit(), so it wouldn't really
be verbose.

This came up because when starting to read the code I had the question about
whether the state in ReservationTracker and the state in these counters were
always in sync. The DCHECKs answered those questions (so leave them if you
keep the state separate), but then was thinking it would be nice to eliminate
the redundant state altogether. That is, the motivation isn't to get rid of
the DCHECKs but to eliminate the redundancy in state (which makes it easier to
understand for the same reason we eliminate the DCHECKs -- there's no state
redundancy).

I wasn't sure if you were shadowing the sate to avoid the atomic. But note
that atomic load/store is unlocked, so is just as fast. So I don't see this as
a motivation for mirroring the state. And I don't see it weirder than mirroring
the state -- we're still using both both locks and atomic variable techniques
one way or the other.

I don't feel too strongly about it but just seems to me that eliminating state
redundancy (and pushing down what we can't eliminate) is clearest. Why do you
feel it's cleaner to have the redundant state?

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

Line 44: /// reservation (and perhaps increase reservations all the way to the
root of the tree).
> Clarified that it was inclusive of resources that are currently used. I thi
yes

PS8, Line 79: .
> Denominating everything in bytes should go a long way to eliminating that c
Thanks.

PS8, Line 85: MemTracker* mem_tracker
> Yes, it will be typically NULL since we need to enforce the limit one level
Great.

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

Line 121: void AllocateFrom(int64_t amount);
Still not clear to me if we need both variants. But we can leave for now and
I'll look again once the client code comes in.

PS9, Line 130: cu
before calling this method (i.e. clarify it's not a global invariant or post
condition. it's a precondition).

PS9, Line 134:
before calling this method.

PS9, Line 137: en.
memory (now that we're being a bit less abstract)

PS9, Line 169: es
what does this do if it's false?

PS9, Line 179:
For the topmost link, return false if ...

Line 196: /// All non-NULL if 'initialized_' is true and a RuntimeProfile was
provided during
profile counters

PS9, Line 200: es not c
profile counters

PS9, Line 217: urrent reservation. 'reservation_' <= 'reservation_limit_
update

Line 231
with this simplification, is this worth it, or should we just do
parent_->mem-tracker_?

http://gerrit.cloudera.org:8080/#/c/3993/9/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

Line 499
why is this header the right place (as opposed to runtime-profile.h)?

http://gerrit.cloudera.org:8080/#/c/3993/9/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

Line 27: #include "util/spinlock.h"
why? i only see indirect references to ObjectPool.

--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Marcel Kornacker
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-09-01 Thread Tim Armstrong (Code Review)

Tim Armstrong has posted comments on this change.

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

Patch Set 8:

(1 comment)

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

PS8, Line 35: Counter
> I see. We could hide this detail inside of this class by defining an SetRe
There's an UpdateReservation() method I added in the last patchset that does
more-or-less what you're suggesting unless there's some subtlety that I'm
missing.

I could remove the DCHECKs for the counter values - those might be overkill.
The main reason I added them initially was because you get some weird behaviour
if you have trackers sharing the same profile and therefore the same counters.
I already have a DCHECK to check for that when adding so should be less of an
issue.

I'm ok with changing to use the counters if you prefer, but not sure if it
works out cleaner.

It feels a little weird to use the atomic operations to read member variables
protected by a lock but ultimately it may not matter that much. It might make
the code a bit more verbose too (i.e. ->value() everywhere).

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

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-31 Thread Dan Hecht (Code Review)

Dan Hecht has posted comments on this change.

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


Patch Set 9:

(1 comment)

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

PS8, Line 35: 
> Will change to the subclass.
I see.  We could hide this detail inside of this class by defining an 
SetReservation(), SetUsedReservation(), and then it's moved out of the way of 
the ReservationTracker code which is good since that's where the interesting 
stuff happens.  And it's one less invariant that it has to check.

Also, now that we never have NULL counters, why not getting rid of the 
reservation_, used_reservation_ and reservation_limit_ fields of 
ReservationTracker() and instead call into this class to get them.  That would 
eliminate another invariant to verify and streamline the amount of state 
further.  What do you think?


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-31 Thread Tim Armstrong (Code Review)

Tim Armstrong has posted comments on this change.

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


Patch Set 8:

(25 comments)

http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/bufferpool/buffer-pool.h
File be/src/bufferpool/buffer-pool.h:

PS8, Line 266: s
> given the way we use this term in reservation-tracker, seems this should be
Done


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

Line 28:   /// This is set even if the reservation was not granted.
> why is that?
Seems more useful for debugging in the case when the reservation isn't granted. 
Otherwise we don't have any info in the profile about the reservation it tried 
to grab. We don't lose any info by capturing it.


PS8, Line 35: Counter
> these peak ones look like they're actually HighWaterMarkCounters.
Will change to the subclass.

HighWaterMarkCounter doesn't report the current value in the profile. I agree 
its redundant but I don't see an easy fix without significant changes to the 
runtime profiles to have multi-valued counters.


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

PS8, Line 40: and accounted against the tracker
: /// directly
> this is kind of vague.
Done


Line 44: /// reservation (and perhaps increase reservations all the way to the 
root of the tree).
> given the various interpretations of "reservation" that we discussed, we ne
Clarified that it was inclusive of resources that are currently used. I think 
that was the main ambiguity, right?


Line 55: /// child_reservations + used_reservation <= reservation.
> maybe just introduce "unused_reservation" here so this can be equality.
Done


Line 56: ///
> let's have a quick explanation of thread-safety rules
Done


PS8, Line 58: can
> will
Done


PS8, Line 58: integration
> (and MemTracker)
Done


PS8, Line 62:  included in memory limit checks
> this could use some updating/clarification
Tried to clarify this.


PS8, Line 69: assume
> require that
Done


PS8, Line 71: assume
> require
Done


Line 73: /// ancestors is also linked to a MemTracker "B", then "B" must be an 
ancestor of "A".
I made this stricter - no gaps in the hierarchy. We can just add in 
ReservationTrackers at the fragment level to make this work.


PS8, Line 79: .
> I know you want to keep ReservationTracker for resources in general (as opp
Denominating everything in bytes should go a long way to eliminating that 
confusion. I'm fine with using bytes, someone else can probably deal with the 
problem down the road if this turns out the be the right abstraction for some 
other resource.


PS8, Line 83: counters_
> the public interface shouldn't really refer to private stuff.  just explain
Done


PS8, Line 85: MemTracker* mem_tracker
> given our requirement that the top most link is the one that enforces limit
Yes, it will be typically NULL since we need to enforce the limit one level 
down at the query mem trackers. I actually removed this parameter so that it is 
always NULL since we don't need it.


PS8, Line 103: amount
> also confusing to use without knowing units
Went through and changed 'amount' to 'bytes' where applicable and reworded if 
appropriate.


PS8, Line 109: 'amount' is available to use
> there is 'amount' of unused reservation.  (for consistency of terminology).
Done


PS8, Line 123: back to the reservation
> this can be confusing because it can be interpretted as incrementing reserv
Used that but said "release" instead of "free" to be consistent with the method 
name.


PS8, Line 127: memory
> here you say memory (which I'm fine with, but let's be consistent one way o
Done


PS8, Line 134: or
> also hard to parse whether the second condition is included or not in "unus
Done


Line 134:   /// Returns the amount of the reservation not used or given to 
childrens' reservations.
> at this tracker
Done


Line 148:   /// Initializes 'counters_', storing the counters in 'profile'.
> Wasn't sure what this meant without reading the code.  How about:
Updated to reflect the dummy profile change.


http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

PS8, Line 57: COUNTER_SET_IF_NOT_NULL
> rather than adding these (and having to know to use them), why not always h
Done. It's a little tricky since there is no RuntimeProfile at the query level. 
MemTracker has a local_counter_ field to work around this.

To get around this, I created a DummyProfile class that can be used to create a 
dummy profile to hold the counters.


PS8, Line 77: COUNTER_ADD_NOT_NULL
> this doesn't look needed
Done


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

Gerrit-MessageType: comment
Ger

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-31 Thread Tim Armstrong (Code Review)

Tim Armstrong has uploaded a new patch set (#9).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool. The reservations are always guaranteed and any buffer/page
allocations require a reservation. Reservations are tracked via a
hierarchy of ReservationTrackers.

Eventually ReservationTrackers should become the main mechanism for
memory accounting, once all runtime memory is handled by the buffer
pool. In the meantime, query/process limits are enforced and memory
is reported through the preexisting MemTracker hierarchy.

Reservations are accounted as consumption against a MemTracker because
the memory is committed and cannot be used for other purposes. The
MemTracker then uses the counters from the ReservationTracker to log
information about buffer-pool memory down to the operator level.

Testing:
Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker-counters.h
A be/src/bufferpool/reservation-tracker-test.cc
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile.h
13 files changed, 1,433 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/9
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-29 Thread Dan Hecht (Code Review)

Dan Hecht has posted comments on this change.

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

Patch Set 8:

(26 comments)

Focused on the public interface.

http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/bufferpool/buffer-pool.h
File be/src/bufferpool/buffer-pool.h:

PS8, Line 266: s
given the way we use this term in reservation-tracker, seems this should be
singular.

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

Line 28: /// This is set even if the reservation was not granted.
why is that?

PS8, Line 31: in bytes
here you do use units, while in ReservationTracker you try to stay resource
agnostic (I prefer this where units are specified -- see comments in
reservation-tracker.h as well).

PS8, Line 35: Counter
these peak ones look like they're actually HighWaterMarkCounters.

Also, why have these peak counters, rather than just having the 'reservation'
counter by a high water mark counter that keeps track of this peak?

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

PS8, Line 40: and accounted against the tracker
: /// directly
this is kind of vague.

PS8, Line 41: its
the child's
to disambiguate.

Line 44: /// reservation (and perhaps increase reservations all the way to the
root of the tree).
given the various interpretations of "reservation" that we discussed, we need
to spell out the various terms more carefully.

Line 55: /// child_reservations + used_reservation <= reservation.
maybe just introduce "unused_reservation" here so this can be equality.

Line 56: ///
let's have a quick explanation of thread-safety rules

PS8, Line 58: can
will

PS8, Line 58: integration
(and MemTracker)

PS8, Line 62: included in memory limit checks
this could use some updating/clarification

PS8, Line 69: assume
require that

PS8, Line 71: assume
require

PS8, Line 79: .
I know you want to keep ReservationTracker for resources in general (as opposed
to memory specific), but we need some way to specify the units (readers won't
know if it's bytes or buffers, etc)

PS8, Line 83: counters_
the public interface shouldn't really refer to private stuff. just explain
what it adds to the profile.

PS8, Line 85: MemTracker* mem_tracker
given our requirement that the top most link is the one that enforces limit, is
this allowed to be non-NULL?

PS8, Line 103: amount
also confusing to use without knowing units

PS8, Line 109: 'amount' is available to use
there is 'amount' of unused reservation. (for consistency of terminology).

PS8, Line 123: back to the reservation
this can be confusing because it can be interpretted as incrementing
reservation by 'account' (if we had used the alternate terminology). How about:

Free up 'amount' of previously allocated resources. The used reservation is
decreased by 'amount'.

PS8, Line 127: memory
here you say memory (which I'm fine with, but let's be consistent one way or
another).

Line 134: /// Returns the amount of the reservation not used or given to
childrens' reservations.
at this tracker

PS8, Line 134: or
also hard to parse whether the second condition is included or not in "unused
reservation". how about:

... not used nor given to children's reservations.

Line 148: /// Initializes 'counters_', storing the counters in 'profile'.
Wasn't sure what this meant without reading the code. How about:

If 'profile' is not NULL, adds this tracker's set of runtime profile counters
to 'profile'.

http://gerrit.cloudera.org:8080/#/c/3993/8/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

PS8, Line 57: COUNTER_SET_IF_NOT_NULL
rather than adding these (and having to know to use them), why not always have
valid runtime counters?

PS8, Line 77: COUNTER_ADD_NOT_NULL
this doesn't look needed

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

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-25 Thread Tim Armstrong (Code Review)

Tim Armstrong has posted comments on this change.

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


Patch Set 7:

(1 comment)

Added reservation counters and reporting, plus simplified the linkage between 
the two hierarchies a little by assuming limit checks only happen at the top 
level.

http://gerrit.cloudera.org:8080/#/c/3993/7/be/src/bufferpool/buffer-pool.h
File be/src/bufferpool/buffer-pool.h:

Line 234:   const int64_t min_page_len_;
I missed changing this to min_buffer_len_.


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-25 Thread Tim Armstrong (Code Review)

Tim Armstrong has uploaded a new patch set (#8).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool. The reservations are always guaranteed and any buffer/page
allocations require a reservation. Reservations are tracked via a
hierarchy of ReservationTrackers.

Eventually ReservationTrackers should become the main mechanism for
memory accounting, once all runtime memory is handled by the buffer
pool. In the meantime, query/process limits are enforced and memory
is reported through the preexisting MemTracker hierarchy.

Reservations are accounted as consumption against a MemTracker because
the memory is committed and cannot be used for other purposes. The
MemTracker then uses the counters from the ReservationTracker to log
information about buffer-pool memory down to the operator level.

Testing:
Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker-counters.h
A be/src/bufferpool/reservation-tracker-test.cc
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/util/runtime-profile-counters.h
12 files changed, 1,440 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/8
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-25 Thread Tim Armstrong (Code Review)

Tim Armstrong has uploaded a new patch set (#7).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool. The reservations are always guaranteed and any buffer/page
allocations require a reservation. Reservations are tracked via a
hierarchy of ReservationTrackers.

Eventually ReservationTrackers should become the main mechanism for
memory accounting, once all runtime memory is handled by the buffer
pool. In the meantime, query/process limits are enforced and memory
is reported through the preexisting MemTracker hierarchy.

Reservations are accounted as consumption against a MemTracker because
the memory is committed and cannot be used for other purposes. The
MemTracker then uses the counters from the ReservationTracker to log
information about buffer-pool memory down to the operator level.

Testing:
Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker-counters.h
A be/src/bufferpool/reservation-tracker-test.cc
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/util/runtime-profile-counters.h
12 files changed, 1,438 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/7
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-22 Thread Tim Armstrong (Code Review)

Tim Armstrong has posted comments on this change.

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


Patch Set 6:

Updated to use the UsedReservation/UnusedReservation terminology I discussed 
offline with reviewers.

-- 
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-22 Thread Tim Armstrong (Code Review)

Tim Armstrong has uploaded a new patch set (#6).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool.

The ReservationTrackers integrate with MemTrackers by counting
reservations as consumption against a MemTracker. This reflects that
fact that the reservations are 'hard': the memory is committed and
cannot be used for other purposes.

Includes new reservation mechanism where reservations are always
guaranteed and buffers can't be pinned without a reservation.
Reservations are tracked via a hierarchy of ReservationTrackers.

Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.h
M be/src/util/uid-util.h
9 files changed, 1,061 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/6
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-17 Thread Tim Armstrong (Code Review)

Tim Armstrong has posted comments on this change.

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


Patch Set 4:

Unfortunately I wasn't really able to address many of the comments. Maybe we 
should chat to clear up some of the issues.

-- 
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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-17 Thread Tim Armstrong (Code Review)

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-17 Thread Tim Armstrong (Code Review)

Tim Armstrong has uploaded a new patch set (#5).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool.

The ReservationTrackers integrate with MemTrackers by counting
reservations as consumption against a MemTracker. This reflects that
fact that the reservations are 'hard': the memory is committed and
cannot be used for other purposes.

Includes new reservation mechanism where reservations are always
guaranteed and buffers can't be pinned without a reservation.
Reservations are tracked via a hierarchy of ReservationTrackers.

Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.h
M be/src/util/uid-util.h
9 files changed, 1,057 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/5
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-17 Thread Marcel Kornacker (Code Review)

Marcel Kornacker 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 from 
reservation'? i find the former more intuitive (as in "i reserved a total of 
10gb and now have 6gb left of my reservation; i deducted 4gb since i obtained 
my initial 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 what 
is left of my reservation).


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 
reservation for a running query (because a discrepancy is interesting from a 
diagnostic perspective)


Line 60: /// therefore always be used without increasing consumption and with 
no risk of exceeding
this seems to contradict what you said earlier.


Line 70:   void InitRootTracker(MemTracker* mem_tracker, int64_t reservation);
could the root be expressed as having parent == null?


Line 74:   /// children will be counted as consumption against 'mem_tracker'.
if the reservation is really counted immediately against a memtracker (without 
actual memory being allocated), doesn't that make the memtracker semantically 
simply a reservation tracker?


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 
'limit' by 'reservation'?


Line 118:   int64_t GetUsage();
i find this term ambiguous as well, because it seems to imply the memory is 
actively being used (which the reservation tracker shouldn't be concerned with).


-- 
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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has posted comments on this change.

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


Patch Set 3:

(7 comments)

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

PS3, Line 32: e.g. bytes of memory
> is this really "e.g." and "resources" given the tight coupling with MemTrac
The coupling with MemTracker is optional: if the mem_tracker is NULL, 
everything works. There's no reason you couldn't use the code to account for 
some other resource if mem_tracker_ is NULL.

I've gone back and forth about this. I don't usually like speculative 
generality, but there's very little about the code or abstraction that's 
specific to memory reservations. I think it would be unfortunate if we have a 
need in the future to track other resource reservations and had to reinvent the 
wheel. I'm happy to change this.


Line 39: /// directly, or distributed to children of the tracker for its own 
reservation.
> are there cases where a client would use reservation directly from a non-le
If an exec node subdivides its allocation using ReservationTrackers below the 
node-level tracker. I think it naturally occurs there, since you would usually 
count some usage again the exec node's tracker.


PS3, Line 41: consume
> to avoid confusion with memtracker consume/release, how about using the ter
Done. Missed it on an earlier pass.


PS3, Line 51: consumption
> usage
Done


PS3, Line 55: optionally
> what cases do we not want to have an associated MemTracker?
I noticed the same thing about the invariant. Hopefully the new version is 
better.

I don't think it makes sense to associate the root MemTracker and 
ReservationTracker right now, since the root MemTracker usage is derived from a 
metric value rather than maintained in the usual way. Eventually if all the 
memory is allocated from the buffer pool we could change that (or get rid of 
the MemTrackers entirely).

Also for ReservationTrackers below the node-level ReservationTracker. E.g. if 
we're allocating temporary trackers to subdivide the reservation. I think it 
would be inconvenient to allocate many corresponding MemTrackers.


PS3, Line 79: InitChildTracker
> For InitRootTracker/InitChildTracker(), maybe these should be static method
Right, it's already enforced with a DCHECK.

The main reason I did it this way is to avoid having to make another heap 
allocation and decide whether it should return a raw/unique/scoped/whatever 
pointer. I don't feel strongly, I think it works ok either way, so I could 
switch it to return a unique_ptr (or something like that).


PS3, Line 83: The reservation must be completely
:   /// released before calling Close().
> This seems to contradict the previous sentence.  Oh, I guess you mean the r
Done


-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has uploaded a new patch set (#4).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool.

The ReservationTrackers integrate with MemTrackers by counting
reservations as consumption against a MemTracker. This reflects that
fact that the reservations are 'hard': the memory is committed and
cannot be used for other purposes.

Includes new reservation mechanism where reservations are always
guaranteed and buffers can't be pinned without a reservation.
Reservations are tracked via a hierarchy of ReservationTrackers.

Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.h
M be/src/util/uid-util.h
9 files changed, 1,058 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/4
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

2016-08-15 Thread Dan Hecht (Code Review)

Dan Hecht has posted comments on this change.

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


Patch Set 3:

(7 comments)

Some initial comments. I think if you clean up some of the terminology it will 
be easier to follow and do the rest of the review.

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

PS3, Line 32: e.g. bytes of memory
is this really "e.g." and "resources" given the tight coupling with MemTracker? 
 Maybe we should just say "memory reservations" to be clearer about what this 
is.


Line 39: /// directly, or distributed to children of the tracker for its own 
reservation.
are there cases where a client would use reservation directly from a non-leaf 
node in the hierarchy?


PS3, Line 41: consume
to avoid confusion with memtracker consume/release, how about using the term 
"use" (to go along with IncreaseUsage()/DecreaseUsage())?


PS3, Line 51: consumption
usage


PS3, Line 55: optionally
what cases do we not want to have an associated MemTracker?
and it seems the last two can be specified as a more precise single invariant 
(though I'm not sure since I don't quite follow the last one).


PS3, Line 79: InitChildTracker
For InitRootTracker/InitChildTracker(), maybe these should be static methods 
that also construct the ReservationTracker() and then make the constructor 
private.  That way it's clear that each reservation tracker should be 
initialized to exactly one of these "personalities" (I assume that's the case).


PS3, Line 83: The reservation must be completely
:   /// released before calling Close().
This seems to contradict the previous sentence.  Oh, I guess you mean the 
reservation usage must be returned before calling Close()?


-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has posted comments on this change.

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


Patch Set 3:

I cut back this patch so that it doesn't actually include the bulk of the 
buffer pool implementation, just client registration and the ReservationTracker 
stuff. I'm going to rework the buffer pool implementation separately.

All the unit tests pass.

-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool

Tim Armstrong has uploaded a new patch set (#3).

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

IMPALA-3201: reservation implementation for new buffer pool

This patch implements the reservation bookkeeping logic for the new
buffer pool.

The ReservationTrackers integrate with MemTrackers by counting
reservations as consumption against a MemTracker. This reflects that
fact that the reservations are 'hard': the memory is committed and
cannot be used for other purposes.

Includes new reservation mechanism where reservations are always
guaranteed and buffers can't be pinned without a reservation.
Reservations are tracked via a hierarchy of ReservationTrackers.

Includes basic tests for buffer pool client registration and various
reservation operations.

Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
---
M be/CMakeLists.txt
A be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-pool-test.cc
A be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
A be/src/bufferpool/reservation-tracker.cc
A be/src/bufferpool/reservation-tracker.h
M be/src/runtime/mem-tracker.h
M be/src/util/uid-util.h
9 files changed, 1,058 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/3993/3
-- 
To view, visit http://gerrit.cloudera.org:8080/3993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3201: reservation implementation for new buffer pool