[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 23 Jan 2018 22:19:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..

IMPALA-4132: Use -fno-omit-frame-pointer

Using -fno-omit-frame-pointer would keep the frame pointers for
functions in the register. As a result we expect more useful stack
traces to be produced.

For testing performance benchmark was executed on TPC-H and TPC-DS
without any significant discrepancy from the baseline results.
(For the specific measures check the attachments in the Jira.)

Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Reviewed-on: http://gerrit.cloudera.org:8080/8612
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/CMakeLists.txt
1 file changed, 2 insertions(+), 1 deletion(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 3:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 23 Jan 2018 18:38:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 23 Jan 2018 18:38:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 2:

I'll go ahead and merge this unless there are any remaining concerns.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 23 Jan 2018 00:41:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 2: Code-Review+1

> Patch Set 2:
>
> Checked the binary sizes for release builds:
>
> Binary size of impalad (output of ls in bytes):
> -fno-omit-frame-pointers: 494185576
> -fomit-frame-pointers:491523224
>
> size of the whole be/build/debug dir (output of du in kbytes):
> -fno-omit-frame-pointers: 51365652
> -fomit-frame-pointers:51087284

Thanks for the update. I think all open questions are answered now.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 22 Jan 2018 16:58:21 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Gabor Kaszab (Code Review)]

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 2:

Checked the binary sizes for release builds:

Binary size of impalad (output of ls in bytes):
-fno-omit-frame-pointers: 494185576
-fomit-frame-pointers:491523224

size of the whole be/build/debug dir (output of du in kbytes):
-fno-omit-frame-pointers: 51365652
-fomit-frame-pointers:51087284


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 22 Jan 2018 15:59:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 2:

I played around a bit with inducing crashes in my dev environment with 
partially stripped impalad binaries (--strip-debug). It seems like breakpad 
does a reasonable job of resolving the stack trace as long as you have symbols 
extracted from a binary (even the --strip-debug option).

I picked an arbitrary spot to crash at though so not sure if it is just an easy 
one or if that will generalise.

However, if we don't have the symbols, without the frame pointer 
minidump_stackwalk can't unwind the stack at all. With the frame pointer, it 
reliably unwinds the stack, although it can't symbolise it because it doesn't 
have symbols.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 12 Dec 2017 02:02:00 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 2:

> Patch Set 2:
>
> I spoke to Gabor about this a few days ago. I agree it would be good to 
> validate it but we we didn't plan for that - the JIRA didn't have a 
> reproduction of a bad stacktrace scenario and the comments on the JIRA were 
> confident that it would be beneficial.
>
> We could try building a release binary, stripping it with strip --strip-debug 
> and inducing a crash. @Lars do you have any examples of crashes from 
> minidumps where we got a "bad" stacktrace?

I couldn't think of a particular case that I could share. However, Breakpad 
first tries to use call frame information that it had extracted from the DWARF 
info during symbol extraction.

 2  impalad!impala::HandleSignal [minidump.cc : 101 + 0x1e]
rbx = 0x   rbp = 0x799871f0
rsp = 0x799871e0   r12 = 0x79987860
r13 = 0x0dc50040   r14 = 0x
r15 = 0x   rip = 0x01a74959
Found by: call frame info

If that fails, it tries to use the frame pointer:

 1  impalad!impala::SleepForMs(long) [time.cc : 30 + 0x1f]
rbp = 0x7f365e6b3aa0   rsp = 0x7f365e6b3a80
rip = 0x01ac61e3
Found by: previous frame's frame pointer

If that fails, it falls back to scanning the stack for addresses that look like 
return addresses:

 8  impalad!boost::detail::function::void_function_invoker0::invoke(boost::detail::function::function_buffer&) [function_template.hpp 
: 112 + 0x6]
rsp = 0x7f365eeb4b00   rip = 0x0163f151
Found by: stack scanning

Resolving a minidump written by a stripped release binary should not show 
"stack scanning" for Impala's frames.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 07 Dec 2017 19:12:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 2:

I spoke to Gabor about this a few days ago. I agree it would be good to 
validate it but we we didn't plan for that - the JIRA didn't have a 
reproduction of a bad stacktrace scenario and the comments on the JIRA were 
confident that it would be beneficial.

We could try building a release binary, stripping it with strip --strip-debug 
and inducing a crash. @Lars do you have any examples of crashes from minidumps 
where we got a "bad" stacktrace?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 07 Dec 2017 00:18:05 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 2:

> Patch Set 1:
>
> (2 comments)

I found out recently that -XX:+PreserveFramePointer exists for JDK8+. It's a 
little bit similar in spirit to this review. No action required, but if we're 
finding that stack frames look great with this change, except for Java stuff, 
we could try that one too!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 29 Nov 2017 17:05:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt
File be/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt@42
PS1, Line 42: #  -fno-omit-frame-pointers: Keep frame pointer for functions. 
Produces better
> I didn't do any deep research into it. It's unclear to me what tooling supp
Have we at least verified that adding frame pointers to release builds does 
improve backtraces?


http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt@42
PS1, Line 42: #  -fno-omit-frame-pointers: Keep frame pointer for functions. 
Produces better
: #   stack traces.
> Done
Is that release build?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 29 Nov 2017 01:10:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt
File be/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt@42
PS1, Line 42: #  -fno-omit-frame-pointers: Keep frame pointer for functions. 
Produces better
> From my side no such consideration was made.
I didn't do any deep research into it. It's unclear to me what tooling supports 
that, whether it gets stripped out when binaries are stripped, etc. It may also 
be a good option, but I'm in favour of moving forward with the frame pointer 
approach since we know that it is widely supported and doesn't appear to have a 
significant perf impact.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 29 Nov 2017 00:08:29 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Gabor Kaszab (Code Review)]

Hello Bharath Vissapragada, Lars Volker, Tim Armstrong, Mostafa Mokhtar, Dan 
Hecht,

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

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

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

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..

IMPALA-4132: Use -fno-omit-frame-pointer

Using -fno-omit-frame-pointer would keep the frame pointers for
functions in the register. As a result we expect more useful stack
traces to be produced.

For testing performance benchmark was executed on TPC-H and TPC-DS
without any significant discrepancy from the baseline results.
(For the specific measures check the attachments in the Jira.)

Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
---
M be/CMakeLists.txt
1 file changed, 2 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8612/1//COMMIT_MSG@10
PS1, Line 10: As a result we expect more useful stack
: traces to be produced.
Just curious how well this works, could you paste a couple of thread stacks 
before and after this change using a stripped binary. It'd be a great change 
from a supportability POV if we can get reliable stacks without having to 
attach symbols separately.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 22 Nov 2017 17:33:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8612/1//COMMIT_MSG@13
PS1, Line 13: For testing performance benchmark was executed on TPC-H and TPC-DS
Did you also test that GDB and breakpad symbol resolution work? Ideally we 
should validate this on various platforms we want to support, and we could use 
this opportunity to add automated tests for both, too.


http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt
File be/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt@42
PS1, Line 42: #  -fno-omit-frame-pointers: Keep frame pointer for functions. 
Produces better
Have you considered using -fasynchronous-unwind-tables instead? This page reads 
like it would do what we want: 
https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:07:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt
File be/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt@42
PS1, Line 42: #  -fno-omit-frame-pointers: Keep frame pointer for functions. 
Produces better
: #   stack traces.
How did this affect the size of the resulting binaries?

nit: I don't think it's accurate to claim that this produces better stack 
traces - it merely helps with symbolization. Should we just remove the 2nd 
sentence. Most people will know what this does, or can just google it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:45:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8612 )

Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..


Patch Set 1: Code-Review+1

(1 comment)

The perf results looked acceptable to me - there didn't seem to be any 
significant regressions.

http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt
File be/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/8612/1/be/CMakeLists.txt@42
PS1, Line 42: pointers
nit: extra s



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:38:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Gabor Kaszab (Code Review)]

Gabor Kaszab has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8612


Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..

IMPALA-4132: Use -fno-omit-frame-pointer

Using -fno-omit-frame-pointer would keep the frame pointers for
functions in the register. As a result we expect more useful stack
traces to be produced.

For testing performance benchmark was executed on TPC-H and TPC-DS
without any significant discrepancy from the baseline results.
(For the specific measures check the attachments in the Jira.)

Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
---
M be/CMakeLists.txt
1 file changed, 3 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab