You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Yida Wu (Code Review)" <ge...@cloudera.org> on 2022/12/19 07:29:03 UTC

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Yida Wu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19377


Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................

IMPALA-11786: Preserve memory for codegen cache

IMPALA-11470 adds support for codegen cache, however the admission
controller is not aware of the memory usage of the codegen cache,
while the codegen cache is actually using the memory quota from
the query memory. It could result in query failures when running
heavy workloads and admission controller has fully admitted queries.

This patch subtracts the codegen cache capacity from the admit
memory limit during initialization, therefore preserve the memory
consumption of codegen cache since the beginning, and treat it as
a separate memory independent to the query memory reservation.

Also changes some failed testcases due to the reduction of the
the admit memory limit.

Tests:
Passed exhaustive tests.

Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
---
M be/src/runtime/exec-env.cc
M tests/custom_cluster/test_admission_controller.py
M tests/custom_cluster/test_executor_groups.py
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 34 insertions(+), 22 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/19377/1
-- 
To view, visit http://gerrit.cloudera.org:8080/19377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................

IMPALA-11786: Preserve memory for codegen cache

IMPALA-11470 adds support for codegen cache, however the admission
controller is not aware of the memory usage of the codegen cache,
while the codegen cache is actually using the memory quota from
the query memory. It could result in query failures when running
heavy workloads and admission controller has fully admitted queries.

This patch subtracts the codegen cache capacity from the admission
memory limit during initialization, therefore preserving the memory
consumption of codegen cache from the beginning, and treating it as
a separate memory independent to the query memory reservation.

Also reduces the max codegen cache memory from 20 percent to 10
percent, and changes some failed testcases due to the reduction of
the admit memory limit.

Tests:
Passed exhaustive tests.

Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Reviewed-on: http://gerrit.cloudera.org:8080/19377
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/exec-env.cc
M tests/custom_cluster/test_admission_controller.py
M tests/custom_cluster/test_executor_groups.py
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 40 insertions(+), 25 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 3: Code-Review+1

The code looks good to me, but similarly to Michael I am also concerned about using 20% of the memory for codegen.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 03 Jan 2023 15:51:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

Looks good!

http://gerrit.cloudera.org:8080/#/c/19377/1/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/19377/1/be/src/runtime/exec-env.cc@337
PS1, Line 337: is_percent
> If the is_percent is true, the codegen_cache_capacity should be 0 because r
I see. Maybe add a DCHECK(!is_percent) here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 22 Dec 2022 15:29:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8951/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 06 Jan 2023 01:21:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 1:

(6 comments)

Looks good to me!

http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG@15
PS1, Line 15: admit
nit. admission


http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG@16
PS1, Line 16: preserve
nit. preserving


http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG@17
PS1, Line 17: and treat
nit. by treating


http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG@17
PS1, Line 17: since
nit. from


http://gerrit.cloudera.org:8080/#/c/19377/1/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/19377/1/be/src/runtime/exec-env.cc@337
PS1, Line 337: is_percent
Do we need to check the value of is_percent after the call?


http://gerrit.cloudera.org:8080/#/c/19377/1/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:

http://gerrit.cloudera.org:8080/#/c/19377/1/tests/custom_cluster/test_executor_groups.py@591
PS1, Line 591: 20%
It would be better to calculate the 3.2GB from 4Gb and 20%, so that one can specify 10% or 30% to fine tune the size of the codegen cache for a workload.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Comment-Date: Tue, 20 Dec 2022 14:50:52 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 1: Code-Review+1

This seems fair. I have some concerns about setting aside 20% of initial memory potentially impacting customers that were expecting their existing queries to continue working. I'm not sure what default memory configuration looks like though.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Comment-Date: Mon, 19 Dec 2022 17:38:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................

IMPALA-11786: Preserve memory for codegen cache

IMPALA-11470 adds support for codegen cache, however the admission
controller is not aware of the memory usage of the codegen cache,
while the codegen cache is actually using the memory quota from
the query memory. It could result in query failures when running
heavy workloads and admission controller has fully admitted queries.

This patch subtracts the codegen cache capacity from the admission
memory limit during initialization, therefore preserving the memory
consumption of codegen cache from the beginning, and treating it as
a separate memory independent to the query memory reservation.

Also changes some failed testcases due to the reduction of the
the admit memory limit.

Tests:
Passed exhaustive tests.

Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
---
M be/src/runtime/exec-env.cc
M tests/custom_cluster/test_admission_controller.py
M tests/custom_cluster/test_executor_groups.py
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 38 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12079/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 02 Jan 2023 17:25:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12041/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Comment-Date: Mon, 19 Dec 2022 07:48:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................

IMPALA-11786: Preserve memory for codegen cache

IMPALA-11470 adds support for codegen cache, however the admission
controller is not aware of the memory usage of the codegen cache,
while the codegen cache is actually using the memory quota from
the query memory. It could result in query failures when running
heavy workloads and admission controller has fully admitted queries.

This patch subtracts the codegen cache capacity from the admission
memory limit during initialization, therefore preserving the memory
consumption of codegen cache from the beginning, and treating it as
a separate memory independent to the query memory reservation.

Also changes some failed testcases due to the reduction of the
the admit memory limit.

Tests:
Passed exhaustive tests.

Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
---
M be/src/runtime/exec-env.cc
M tests/custom_cluster/test_admission_controller.py
M tests/custom_cluster/test_executor_groups.py
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 35 insertions(+), 22 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12090/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Jan 2023 00:38:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 06 Jan 2023 01:21:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 2:

(6 comments)

Thanks Qifan and Michael. I think the change could affect some existing queries by taking away memory from admission control, but the change is necessary, and the user can disable the codegen cache or lower the value if needs to resume.

http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG@15
PS1, Line 15: admis
> nit. admission
Done


http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG@16
PS1, Line 16: preservi
> nit. preserving
Done


http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG@17
PS1, Line 17: nd treati
> nit. by treating
Done


http://gerrit.cloudera.org:8080/#/c/19377/1//COMMIT_MSG@17
PS1, Line 17: from 
> nit. from
Done


http://gerrit.cloudera.org:8080/#/c/19377/1/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/19377/1/be/src/runtime/exec-env.cc@337
PS1, Line 337: is_percent
> Do we need to check the value of is_percent after the call?
If the is_percent is true, the codegen_cache_capacity should be 0 because relative_reference is 0, then the codegen cache will be disabled.
I think for now we don't support percentage for codegen cache, just like the buffer pool limit below.


http://gerrit.cloudera.org:8080/#/c/19377/1/tests/custom_cluster/test_executor_groups.py
File tests/custom_cluster/test_executor_groups.py:

http://gerrit.cloudera.org:8080/#/c/19377/1/tests/custom_cluster/test_executor_groups.py@591
PS1, Line 591: 20%
> It would be better to calculate the 3.2GB from 4Gb and 20%, so that one can
Added a calculation.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Dec 2022 23:41:56 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 4: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Jan 2023 00:19:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12051/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Comment-Date: Tue, 20 Dec 2022 23:36:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Jan 2023 14:15:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19377/1/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/19377/1/be/src/runtime/exec-env.cc@337
PS1, Line 337: is_percent
> I see. Maybe add a DCHECK(!is_percent) here.
Added.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 02 Jan 2023 17:06:42 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 5: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 06 Jan 2023 06:28:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 3:

The 20% is the maximum that the codegen cache memory can reach, but I think I can lower the maximum to something like 10%.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 03 Jan 2023 17:08:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................

IMPALA-11786: Preserve memory for codegen cache

IMPALA-11470 adds support for codegen cache, however the admission
controller is not aware of the memory usage of the codegen cache,
while the codegen cache is actually using the memory quota from
the query memory. It could result in query failures when running
heavy workloads and admission controller has fully admitted queries.

This patch subtracts the codegen cache capacity from the admission
memory limit during initialization, therefore preserving the memory
consumption of codegen cache from the beginning, and treating it as
a separate memory independent to the query memory reservation.

Also reduces the max codegen cache memory from 20 percent to 10
percent, and changes some failed testcases due to the reduction of
the admit memory limit.

Tests:
Passed exhaustive tests.

Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
---
M be/src/runtime/exec-env.cc
M tests/custom_cluster/test_admission_controller.py
M tests/custom_cluster/test_executor_groups.py
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 40 insertions(+), 25 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 3:

It may be a good idea to test a good default value <D> (say 10%) with a suitable workload such as tpcds. Basically, a minimal value that can help achieve the most benefit of the cache. 

In the long run, it seems a good idea to implement a caching strategy (i.e. removal of old items) so that the cache works well even configured with less than optimal threshold value <D>.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Jan 2023 00:08:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11786: Preserve memory for codegen cache

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19377 )

Change subject: IMPALA-11786: Preserve memory for codegen cache
......................................................................


Patch Set 4:

Reduced the max codegen cache memory to 10%. I think we do have the eviction policy for the cache, the main purpose of max memory percentage is just to prevent the user from setting a extremely large number by accident.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdc04ba1b91578d74684209a11c815225b8505a
Gerrit-Change-Number: 19377
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Jan 2023 00:36:37 +0000
Gerrit-HasComments: No