You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2023/02/16 12:42:13 UTC
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Quanlong Huang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19505
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
IMPALA-11223: Use unique id to create codegen instances
When startup flag asm_module_dir is set, impalad will dump the codegen
disassembly to files under that folder. The file name is "id.asm" in
which "id" is the codegen instance id. Before IMPALA-4080 (f2837e9), we
use fragment instance id as the codegen id. After that, since codegen
are done in fragment level (shared by fragment instances), we use query
id instead. This introduces conflicts between different fragments. The
asm files will be overwritten.
This changes the codegen instance id to be "QueryID_FragmentName_PID".
The PID suffix is needed since we usually have several impalads running
together on our dev box.
Tests:
- Manually verified the asm file names are expected.
Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
---
M be/src/runtime/fragment-state.cc
1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/19505/1
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12544/ : 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/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 06 Mar 2023 01:01:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
Thanks Quanlong!
http://gerrit.cloudera.org:8080/#/c/19505/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19505/2//COMMIT_MSG@12
PS2, Line 12: use
Nit: used
http://gerrit.cloudera.org:8080/#/c/19505/2//COMMIT_MSG@13
PS2, Line 13: are
Nit: is
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 27 Feb 2023 09:44:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Yida Wu, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19505
to look at the new patch set (#3).
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
IMPALA-11223: Use unique id to create codegen instances
When startup flag asm_module_dir is set, impalad will dump the codegen
disassembly to files under that folder. The file name is "id.asm" in
which "id" is the codegen instance id. Before IMPALA-4080 (f2837e9), we
used fragment instance id as the codegen id. After that, since codegen
is done in fragment level (shared by fragment instances), we use query
id instead. This introduces conflicts between different fragments. The
asm files will be overwritten.
The same conflict happens in dumping IR modules (when unopt_module_dir
or opt_module_dir is set).
This changes the codegen instance id to be "QueryID_FragmentName_PID".
The PID suffix is needed since we usually have several impalads running
together on our dev box.
Tests:
- Manually verified the asm file names are expected.
Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
---
M be/src/runtime/fragment-state.cc
1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/19505/3
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9115/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 07 Mar 2023 02:44:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 1: Code-Review+1
This seems fine to me. I don't see any use of the ID outside of writing to disk for debugging. Adding Yida since he's been deep in codegen recently.
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 16 Feb 2023 17:59:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 4:
As discussed with Yida offline, added some logs to show the dumped files. The log will be prepended the instance id which also helps to know which instance performs the codegen.
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 06 Mar 2023 00:44:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
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/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
IMPALA-11223: Use unique id to create codegen instances
When startup flag asm_module_dir is set, impalad will dump the codegen
disassembly to files under that folder. The file name is "id.asm" in
which "id" is the codegen instance id. Before IMPALA-4080 (f2837e9), we
used fragment instance id as the codegen id. After that, since codegen
is done in fragment level (shared by fragment instances), we use query
id instead. This introduces conflicts between different fragments. The
asm files will be overwritten.
The same conflict happens in dumping IR modules (when unopt_module_dir
or opt_module_dir is set).
This changes the codegen instance id to be "QueryID_FragmentName_PID".
The PID suffix is needed since we usually have several impalads running
together on our dev box.
Also adds logs when IR or disassembly are dumped to files. It helps to
know which instance performs the codegen.
Tests:
- Manually verified the asm file names are expected.
Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Reviewed-on: http://gerrit.cloudera.org:8080/19505
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/llvm-codegen.cc
M be/src/runtime/fragment-state.cc
3 files changed, 11 insertions(+), 2 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 6
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 1:
Thanks Quanlong, this seems good to me.
I wonder if this solves a problem I've come across a few times in the past: when I was looking for a codegen'd function in the unoptimised llvm bitcode, sometimes when I ran the same query several times, the function only showed up in about 1 case out of 5. Do you think it's possible that the output file was overwritten by another fragment? Or is this change only relevant for the asm output?
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Feb 2023 10:31:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19505/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19505/2//COMMIT_MSG@12
PS2, Line 12: use
> Nit: used
Done
http://gerrit.cloudera.org:8080/#/c/19505/2//COMMIT_MSG@13
PS2, Line 13: is
> Nit: is
Done
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 28 Feb 2023 10:24:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 3:
> Patch Set 3: Code-Review+1
>
> LGTM!
> Because it changes the id of the LlvmCodeGen, just to confirm do you know is there any other place also needs the id?
I think the only other place that uses this id is 'fn_symbol' generated in CodegenSymbolEmitter:
// Append id to symbol to disambiguate different instances of jitted functions.
string fn_symbol = Substitute("$0:$1", name_or_err.get().data(), id_);
It's used to dump the disassembly when asm_module_dir is set. The new id still meets the requirement of uniqueness.
https://github.com/apache/impala/blob/6ebf35cd5d9d8762147b73bd99b53fd2ffc426d3/be/src/codegen/codegen-symbol-emitter.cc#L109
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 03 Mar 2023 07:57:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12481/ : 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/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 28 Feb 2023 10:43:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 1:
> Patch Set 1:
>
> Thanks Quanlong, this seems good to me.
>
> I wonder if this solves a problem I've come across a few times in the past: when I was looking for a codegen'd function in the unoptimised llvm bitcode, sometimes when I ran the same query several times, the function only showed up in about 1 case out of 5. Do you think it's possible that the output file was overwritten by another fragment? Or is this change only relevant for the asm output?
Yeah, thanks for sharing this! The id is also used in dumping IRs:
https://github.com/apache/impala/blob/d0592c0dbf4d634cc26b2e9872a9f62af5249cdd/be/src/codegen/llvm-codegen.cc#L1258
https://github.com/apache/impala/blob/d0592c0dbf4d634cc26b2e9872a9f62af5249cdd/be/src/codegen/llvm-codegen.cc#L1324
So this patch will also fix the issue you encountered. I'll update the comments about this.
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sun, 26 Feb 2023 01:46:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 07 Mar 2023 07:51:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 3: Code-Review+1
LGTM!
Because it changes the id of the LlvmCodeGen, just to confirm do you know is there any other place also needs the id?
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 03 Mar 2023 03:24:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12446/ : 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/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sun, 26 Feb 2023 02:11:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Yida Wu, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19505
to look at the new patch set (#4).
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
IMPALA-11223: Use unique id to create codegen instances
When startup flag asm_module_dir is set, impalad will dump the codegen
disassembly to files under that folder. The file name is "id.asm" in
which "id" is the codegen instance id. Before IMPALA-4080 (f2837e9), we
used fragment instance id as the codegen id. After that, since codegen
is done in fragment level (shared by fragment instances), we use query
id instead. This introduces conflicts between different fragments. The
asm files will be overwritten.
The same conflict happens in dumping IR modules (when unopt_module_dir
or opt_module_dir is set).
This changes the codegen instance id to be "QueryID_FragmentName_PID".
The PID suffix is needed since we usually have several impalads running
together on our dev box.
Also adds logs when IR or disassembly are dumped to files. It helps to
know which instance performs the codegen.
Tests:
- Manually verified the asm file names are expected.
Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
---
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/codegen/llvm-codegen.cc
M be/src/runtime/fragment-state.cc
3 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/19505/4
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 07 Mar 2023 02:44:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 4: Code-Review+2
Thanks Quanlong for fixing the issue and adding the logs.
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 06 Mar 2023 01:05:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Yida Wu, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19505
to look at the new patch set (#2).
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
IMPALA-11223: Use unique id to create codegen instances
When startup flag asm_module_dir is set, impalad will dump the codegen
disassembly to files under that folder. The file name is "id.asm" in
which "id" is the codegen instance id. Before IMPALA-4080 (f2837e9), we
use fragment instance id as the codegen id. After that, since codegen
are done in fragment level (shared by fragment instances), we use query
id instead. This introduces conflicts between different fragments. The
asm files will be overwritten.
The same conflict happens in dumping IR modules (when unopt_module_dir
or opt_module_dir is set).
This changes the codegen instance id to be "QueryID_FragmentName_PID".
The PID suffix is needed since we usually have several impalads running
together on our dev box.
Tests:
- Manually verified the asm file names are expected.
Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
---
M be/src/runtime/fragment-state.cc
1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/19505/2
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12387/ : 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/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Feb 2023 13:06:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11223: Use unique id to create codegen instances
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19505 )
Change subject: IMPALA-11223: Use unique id to create codegen instances
......................................................................
Patch Set 3: Code-Review+1
Thanks, this is fine for me. Let's see if Yida would like to review it, otherwise I can give +2.
--
To view, visit http://gerrit.cloudera.org:8080/19505
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7672906365c916bbe750eeb9906cab38573e6c31
Gerrit-Change-Number: 19505
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 28 Feb 2023 10:27:14 +0000
Gerrit-HasComments: No