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/21 07:28:31 UTC
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Quanlong Huang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19521
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
IMPALA-11927: Enable frame pointers for snappy and lz4
Adds the -fno-omit-frame-pointer flag while compiling snappy and lz4.
These two libs are used in many different code paths. In perf
investigation, the flame graphs make more sense with them enabled.
Tests:
- Ran perf test on TPCH. No significant perf change is detected.
Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
---
M source/lz4/build.sh
M source/snappy/build.sh
2 files changed, 6 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/21/19521/1
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
Patch Set 1:
(1 comment)
looks good to me
http://gerrit.cloudera.org:8080/#/c/19521/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19521/1//COMMIT_MSG@11
PS1, Line 11: flame
nit: frame?
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 16:50:11 +0000
Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19521/1/source/snappy/build.sh
File source/snappy/build.sh:
http://gerrit.cloudera.org:8080/#/c/19521/1/source/snappy/build.sh@41
PS1, Line 41: fPIC
Shouldn't we also mention the addition of the -fPIC flag in the commit message?
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 14:04:28 +0000
Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19521/1/source/snappy/build.sh
File source/snappy/build.sh:
http://gerrit.cloudera.org:8080/#/c/19521/1/source/snappy/build.sh@41
PS1, Line 41: fPIC
> Shouldn't we also mention the addition of the -fPIC flag in the commit mess
Thanks for catching this. I encountered link errors without -fPIC here. After digging more, I think I understand the reason.
-fPIC is added in CFLAGS and CXXFLAGS exported by init-compiler.sh. As mentioned in the CMake doc, CXXFLAGS will be ignored if the CMAKE_CXX_FLAGS variable is defined. So we lose what we set in CXXFLAGS. I think the correct way of adding the new flag is by
-DCMAKE_CXX_FLAGS="$CXXFLAGS -fno-omit-frame-pointer"
It's strange that I didn't see link errors on liblz4. Anyway, I'll update the patch.
https://cmake.org/cmake/help/v3.22/envvar/CXXFLAGS.html
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Mar 2023 13:03:46 +0000
Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
IMPALA-11927: Enable frame pointers for snappy and lz4
Adds the -fno-omit-frame-pointer flag while compiling snappy and lz4.
These two libs are used in many different code paths, e.g. snappy
decompression is used in several code paths of late materialization,
lz4 is used to compress row batches in KRPC sender.
In perf investigation, the flame graphs make more sense with them
enabled.
Tests:
- Ran perf test on TPCH. No significant perf change is detected.
Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Reviewed-on: http://gerrit.cloudera.org:8080/19521
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
Reviewed-by: Michael Smith <mi...@cloudera.com>
Tested-by: Quanlong Huang <hu...@gmail.com>
---
M source/lz4/build.sh
M source/snappy/build.sh
2 files changed, 13 insertions(+), 5 deletions(-)
Approvals:
Wenzhe Zhou: Looks good to me, but someone else must approve
Michael Smith: Looks good to me, approved
Quanlong Huang: Verified
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19521/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19521/1//COMMIT_MSG@11
PS1, Line 11: flame
> Thanks for clarification
Yeah. I just attached the FlameGraphs with and without frame pointers in libsnappy in the JIRA for comparison.
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 02:19:03 +0000
Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19521/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19521/1//COMMIT_MSG@11
PS1, Line 11: flame
> nit: frame?
I assumed he meant https://www.brendangregg.com/flamegraphs.html.
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 18:18:11 +0000
Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19521
to look at the new patch set (#2).
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
IMPALA-11927: Enable frame pointers for snappy and lz4
Adds the -fno-omit-frame-pointer flag while compiling snappy and lz4.
These two libs are used in many different code paths. In perf
investigation, the flame graphs make more sense with them enabled.
Tests:
- Ran perf test on TPCH. No significant perf change is detected.
Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
---
M source/lz4/build.sh
M source/snappy/build.sh
2 files changed, 13 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/21/19521/2
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19521/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19521/1//COMMIT_MSG@11
PS1, Line 11: flame
> I assumed he meant https://www.brendangregg.com/flamegraphs.html.
Thanks for clarification
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Feb 2023 18:39:50 +0000
Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Mar 2023 01:32:29 +0000
Gerrit-HasComments: No
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Mar 2023 01:01:18 +0000
Gerrit-HasComments: No
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19521
to look at the new patch set (#3).
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
IMPALA-11927: Enable frame pointers for snappy and lz4
Adds the -fno-omit-frame-pointer flag while compiling snappy and lz4.
These two libs are used in many different code paths, e.g. snappy
decompression is used in several code paths of late materialization,
lz4 is used to compress row batches in KRPC sender.
In perf investigation, the flame graphs make more sense with them
enabled.
Tests:
- Ran perf test on TPCH. No significant perf change is detected.
Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
---
M source/lz4/build.sh
M source/snappy/build.sh
2 files changed, 13 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/21/19521/3
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[native-toolchain-CR] IMPALA-11927: Enable frame pointers for snappy and lz4
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19521 )
Change subject: IMPALA-11927: Enable frame pointers for snappy and lz4
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7d767e42a90327ddede53ff6c55f17de05cf52
Gerrit-Change-Number: 19521
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Mar 2023 18:18:59 +0000
Gerrit-HasComments: No