[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 9: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5681: release reservation from blocking operators
..


IMPALA-5681: release reservation from blocking operators

When an in-memory blocking aggregation or join is in the GetNext()
phase where it is outputting accumulated rows then we expect
memory consumption to monotonically decrease because no more
rows will be accumulated in memory.

This change adds support to release unused reservation and makes
use of it for in-memory aggregations and sorts.

We don't release memory for operators with spilled data, since they
may need the reservation to bring it back into memory. We also
don't release memory in subplans, since it will probably be used
in a later iteration of the subplan.

Testing:
Updated spilling test that now requires less memory.

Ran stress test binary search on tpch_parquet. No changes, except
Q18 now requires 325MB instead of 450MB to execute without spilling.

Ran query with two sorts in the same pipeline and watched /memz to
confirm that the first node in the pipeline was incrementally releasing
memory. Added a regression test based on this experiment.

Added a backend test to directly test reservation decreasing.

Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Reviewed-on: http://gerrit.cloudera.org:8080/7619
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/buffer-pool.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/sorter.cc
M be/src/runtime/sorter.h
M testdata/workloads/functional-query/queries/QueryTest/spilling-aggs.test
A testdata/workloads/tpch/queries/sort-reservation-usage.test
M tests/query_test/test_sort.py
14 files changed, 187 insertions(+), 11 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1086/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 8:

Hit clang-tidy warning (dropped Status)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 8: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1083/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 8: Code-Review+2

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 8:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1083/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Hello Thomas Tauber-Marshall, Dan Hecht,

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

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

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

Change subject: IMPALA-5681: release reservation from blocking operators
..

IMPALA-5681: release reservation from blocking operators

When an in-memory blocking aggregation or join is in the GetNext()
phase where it is outputting accumulated rows then we expect
memory consumption to monotonically decrease because no more
rows will be accumulated in memory.

This change adds support to release unused reservation and makes
use of it for in-memory aggregations and sorts.

We don't release memory for operators with spilled data, since they
may need the reservation to bring it back into memory. We also
don't release memory in subplans, since it will probably be used
in a later iteration of the subplan.

Testing:
Updated spilling test that now requires less memory.

Ran stress test binary search on tpch_parquet. No changes, except
Q18 now requires 325MB instead of 450MB to execute without spilling.

Ran query with two sorts in the same pipeline and watched /memz to
confirm that the first node in the pipeline was incrementally releasing
memory. Added a regression test based on this experiment.

Added a backend test to directly test reservation decreasing.

Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/buffer-pool.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/sorter.cc
M be/src/runtime/sorter.h
M testdata/workloads/functional-query/queries/QueryTest/spilling-aggs.test
A testdata/workloads/tpch/queries/sort-reservation-usage.test
M tests/query_test/test_sort.py
14 files changed, 187 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 7: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 6: Code-Review+1

carry +1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 7: Code-Review+1

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Hello Thomas Tauber-Marshall,

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

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

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

Change subject: IMPALA-5681: release reservation from blocking operators
..

IMPALA-5681: release reservation from blocking operators

When an in-memory blocking aggregation or join is in the GetNext()
phase where it is outputting accumulated rows then we expect
memory consumption to monotonically decrease because no more
rows will be accumulated in memory.

This change adds support to release unused reservation and makes
use of it for in-memory aggregations and sorts.

We don't release memory for operators with spilled data, since they
may need the reservation to bring it back into memory. We also
don't release memory in subplans, since it will probably be used
in a later iteration of the subplan.

Testing:
Updated spilling test that now requires less memory.

Ran stress test binary search on tpch_parquet. No changes, except
Q18 now requires 325MB instead of 450MB to execute without spilling.

Ran query with two sorts in the same pipeline and watched /memz to
confirm that the first node in the pipeline was incrementally releasing
memory. Added a regression test based on this experiment.

Added a backend test to directly test reservation decreasing.

Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/buffer-pool.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/sorter.cc
M be/src/runtime/sorter.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
A testdata/workloads/tpch/queries/sort-reservation-usage.test
M tests/query_test/test_sort.py
14 files changed, 187 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7619/5/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

PS5, Line 238:  excess of the node's initial reservation
> why do we need to hold on to the initial reservation if this should only be
Yeah. The real requirement is that at least the initial reservation has to be 
handed back to InitialReservations. It would be correct if 
ReleaseUnusedReservation() reduced it below the minimum reservation by handing 
back the memory to InitialReservations (as opposed to just releasing it).

It seemed simplest for now to avoid that complication in 
ReleaseUnusedReservation()

Documented the invariant in ClaimBufferReservation().


http://gerrit.cloudera.org:8080/#/c/7619/5/be/src/exec/sort-node.cc
File be/src/exec/sort-node.cc:

PS5, Line 106: returned_buffer_
> so is that an optimization, or is it needed for correctness?
It's an optimization to avoid calling this logic each iteration.


Line 113:   DCHECK(status.ok()) << "Should not fail - no runs were spilled. 
"
> to make sure I understand - that's because there should be no unpinned (dir
Yep. Added a DCHECK here and in the agg.


http://gerrit.cloudera.org:8080/#/c/7619/5/be/src/exec/sort-node.h
File be/src/exec/sort-node.h:

PS5, Line 72: last
> does that mean "final" or "previous"?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7619/5/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

PS5, Line 238:  excess of the node's initial reservation
why do we need to hold on to the initial reservation if this should only be 
used after we're done spilling? is that because we need to hold on to that for 
a later operator that we'll "transfer" it to? if so, do we document this 
invariant (that the operator must hold on to the initial reservation until 
Close())?


http://gerrit.cloudera.org:8080/#/c/7619/5/be/src/exec/sort-node.cc
File be/src/exec/sort-node.cc:

PS5, Line 106: returned_buffer_
so is that an optimization, or is it needed for correctness?


Line 113:   DCHECK(status.ok()) << "Should not fail - no runs were spilled. 
"
to make sure I understand - that's because there should be no unpinned (dirty) 
pages? worth dchecking directly that no pages for this client are unpinned, to 
verify the HasSpilledRuns() logic? (since failure on this path would be rare to 
exercise).


http://gerrit.cloudera.org:8080/#/c/7619/5/be/src/exec/sort-node.h
File be/src/exec/sort-node.h:

PS5, Line 72: last
does that mean "final" or "previous"?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 5: Code-Review+1

carry

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7619/4/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

PS4, Line 1203:  
> put a comma here
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Hello Thomas Tauber-Marshall,

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

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

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

Change subject: IMPALA-5681: release reservation from blocking operators
..

IMPALA-5681: release reservation from blocking operators

When an in-memory blocking aggregation or join is in the GetNext()
phase where it is outputting accumulated rows then we expect
memory consumption to monotonically decrease because no more
rows will be accumulated in memory.

This change adds support to release unused reservation and makes
use of it for in-memory aggregations and sorts.

We don't release memory for operators with spilled data, since they
may need the reservation to bring it back into memory. We also
don't release memory in subplans, since it will probably be used
in a later iteration of the subplan.

Testing:
Updated spilling test that now requires less memory.

Ran stress test binary search on tpch_parquet. No changes, except
Q18 now requires 325MB instead of 450MB to execute without spilling.

Ran query with two sorts in the same pipeline and watched /memz to
confirm that the first node in the pipeline was incrementally releasing
memory. Added a regression test based on this experiment.

Added a backend test to directly test reservation decreasing.

Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/buffer-pool.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/sorter.cc
M be/src/runtime/sorter.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
A testdata/workloads/tpch/queries/sort-reservation-usage.test
M tests/query_test/test_sort.py
14 files changed, 163 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7619/4/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

PS4, Line 1203:  
put a comma here


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 4: Code-Review+1

carry +1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7619/3//COMMIT_MSG
Commit Message:

PS3, Line 17: We don't r
> We don't release
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

Hello Thomas Tauber-Marshall,

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

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

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

Change subject: IMPALA-5681: release reservation from blocking operators
..

IMPALA-5681: release reservation from blocking operators

When an in-memory blocking aggregation or join is in the GetNext()
phase where it is outputting accumulated rows then we expect
memory consumption to monotonically decrease because no more
rows will be accumulated in memory.

This change adds support to release unused reservation and makes
use of it for in-memory aggregations and sorts.

We don't release memory for operators with spilled data, since they
may need the reservation to bring it back into memory. We also
don't release memory in subplans, since it will probably be used
in a later iteration of the subplan.

Testing:
Updated spilling test that now requires less memory.

Ran stress test binary search on tpch_parquet. No changes, except
Q18 now requires 325MB instead of 450MB to execute without spilling.

Ran query with two sorts in the same pipeline and watched /memz to
confirm that the first node in the pipeline was incrementally releasing
memory. Added a regression test based on this experiment.

Added a backend test to directly test reservation decreasing.

Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/buffer-pool.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/sorter.cc
M be/src/runtime/sorter.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
A testdata/workloads/tpch/queries/sort-reservation-usage.test
M tests/query_test/test_sort.py
14 files changed, 163 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-5681: release reservation from blocking operators
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7619/3//COMMIT_MSG
Commit Message:

PS3, Line 17: We release
We don't release


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-5681: release reservation from blocking operators
..

IMPALA-5681: release reservation from blocking operators

When an in-memory blocking aggregation or join is in the GetNext()
phase where it is outputting accumulated rows then we expect
memory consumption to monotonically decrease because no more
rows will be accumulated in memory.

This change adds support to release unused reservation and makes
use of it for in-memory aggregations and sorts.

We release memory for operators with spilled data, since they
may need the reservation to bring it back into memory. We also
don't release memory in subplans, since it will probably be used
in a later iteration of the subplan.

Testing:
Updated spilling test that now requires less memory.

Ran stress test binary search on tpch_parquet. No changes, except
Q18 now requires 325MB instead of 450MB to execute without spilling.

Ran query with two sorts in the same pipeline and watched /memz to
confirm that the first node in the pipeline was incrementally releasing
memory. Added a regression test based on this experiment.

Added a backend test to directly test reservation decreasing.

Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/buffer-pool.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/sorter.cc
M be/src/runtime/sorter.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
A testdata/workloads/tpch/queries/sort-reservation-usage.test
M tests/query_test/test_sort.py
14 files changed, 163 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5681: release reservation from blocking operators

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-5681: release reservation from blocking operators
..

IMPALA-5681: release reservation from blocking operators

When an in-memory blocking aggregation or join is in the GetNext()
phase where it is outputting accumulated rows then we expect
memory consumption to monotonically decrease because no more
rows will be accumulated in memory.

This change adds support to release unused reservation and makes
use of it for in-memory aggregations and sorts.

We release memory for operators with spilled data, since they
may need the reservation to bring it back into memory. We also
don't release memory in subplans, since it will probably be used
in a later iteration of the subplan.

Testing:
Updated spilling test that now requires less memory.

Ran stress test binary search on tpch_parquet. No changes, except
Q18 now requires 325MB instead of 450MB to execute without spilling.

Ran query with two sorts in the same pipeline and watched /memz to
confirm that the first node in the pipeline was incrementally releasing
memory. Added a regression test based on this experiment.

Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/bufferpool/buffer-pool.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/sorter.cc
M be/src/runtime/sorter.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
A testdata/workloads/tpch/queries/sort-reservation-usage.test
M tests/query_test/test_sort.py
13 files changed, 112 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/7619/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7619
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f4d0ad127d5fcd14b9821a7c127eec11d98692f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong