Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

.eh_frame: unwind table map improvements #794

Merged
merged 3 commits into from Sep 7, 2022
Merged

.eh_frame: unwind table map improvements #794

merged 3 commits into from Sep 7, 2022

Conversation

javierhonduco
Copy link
Contributor

  • increase table size by 30k items
  • read symbols from the dynamic string table, too
  • add initial code to compact the unwind table by removing redundant rows (disabled by default, will add tests in the near future, but tested with parca-demo-cpp-no-fp and parca-demo-cpp and they both worked)

@javierhonduco
.eh_frame: Increase table size and improve panic message
1ce1d88 
Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
@javierhonduco
.eh_frame: Read symbols from dynamic strings, too
f596729 
Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
@javierhonduco javierhonduco requested a review from a team as a code owner September 7, 2022 14:37
@javierhonduco javierhonduco force-pushed the javier/unwind-table-map-improvements branch from 0686a87 to a6973b6 Compare September 7, 2022 14:38
@javierhonduco
.eh_frame: Remove redundant rows
4f88ac5 
For parca-demo-cpp
```
Compacted the unwind table from 65608 elements to 53153 elements
```

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
@javierhonduco javierhonduco force-pushed the javier/unwind-table-map-improvements branch from a6973b6 to 4f88ac5 Compare September 7, 2022 14:42
kakkoyun
kakkoyun approved these changes Sep 7, 2022
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

pkg/profiler/cpu/cpu.go
@@ -415,6 +417,25 @@ func (p *CPU) ensureUnwindTables(pid int) error {
}
}

if compact {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

pkg/profiler/cpu/cpu.go
prevRBPRegisterOffset := int64(0)

for _, row := range pt {
newEntry := row.CFA.Reg != prevCFARegister || row.CFA.Offset != prevCFAOffset || row.RBP.Offset != prevRBPRegisterOffset
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
newEntry := row.CFA.Reg != prevCFARegister || row.CFA.Offset != prevCFAOffset || row.RBP.Offset != prevRBPRegisterOffset
notSameAsPrev := row.CFA.Reg != prevCFARegister || row.CFA.Offset != prevCFAOffset || row.RBP.Offset != prevRBPRegisterOffset

I think this is semantically more correct, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has changed after I've commented above 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just changing it :D Thanks for noticing, the previous name was outright wrong!

@javierhonduco javierhonduco merged commit 7e8bf38 into ehframe_stack_unwinding Sep 7, 2022
@javierhonduco javierhonduco deleted the javier/unwind-table-map-improvements branch September 7, 2022 14:47
javierhonduco added a commit to parca-dev/testdata that referenced this pull request Sep 29, 2023
@javierhonduco
Remove redundant rows
eb7dde8 
As we binary search over the unwind information, any neighbouring rows
with the same unwind info can be summarised in just one.

This optimisation was obvious from early on and I added it back in
September last year (parca-dev/parca-agent#794)
but it was removed later on.

Test Plan
========

See Agent PR.

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
javierhonduco added a commit to parca-dev/testdata that referenced this pull request Sep 29, 2023
@javierhonduco
Remove redundant rows
7d9b032 
As we binary search over the unwind information, any neighbouring rows
with the same unwind info can be summarised in just one.

This optimisation was obvious from early on and I added it back in
September last year (parca-dev/parca-agent#794)
but it was removed later on.

Test Plan
========

See Agent PR.

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
@javierhonduco javierhonduco mentioned this pull request Sep 29, 2023
Merged
javierhonduco added a commit that referenced this pull request Sep 29, 2023
@javierhonduco
Remove redundant unwind rows
65b6093 
As we binary search over the unwind information, any neighbouring rows
with the same unwind info can be summarised in just one.

This optimisation was obvious from early on and I added it back in September
last year (#794) but it was removed later on and never re-added.

Test Plan
=========

Snapshot tests + added unit tests

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
@javierhonduco javierhonduco mentioned this pull request Sep 29, 2023
Merged
javierhonduco added a commit that referenced this pull request Sep 29, 2023
@javierhonduco
Remove redundant unwind rows
f9a8050 
As we binary search over the unwind information, any neighbouring rows
with the same unwind info can be summarised in just one.

This optimisation was obvious from early on and I added it back in September
last year (#794) but it was removed later on and never re-added.

Test Plan
=========

Snapshot tests + added unit tests

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
javierhonduco added a commit that referenced this pull request Sep 29, 2023
@javierhonduco
Remove redundant unwind rows
12ae8b2 
As we binary search over the unwind information, any neighbouring rows
with the same unwind info can be summarised in just one.

This optimisation was obvious from early on and I added it back in September
last year (#794) but it was removed later on and never re-added.

Test Plan
=========

Snapshot tests + added unit tests

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
javierhonduco added a commit that referenced this pull request Sep 29, 2023
@javierhonduco
Remove redundant unwind rows
3532ee0 
As we binary search over the unwind information, any neighbouring rows
with the same unwind info can be summarised in just one.

This optimisation was obvious from early on and I added it back in September
last year (#794) but it was removed later on and never re-added.

Test Plan
=========

Snapshot tests + added unit tests

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
javierhonduco added a commit that referenced this pull request Sep 29, 2023
@javierhonduco
Remove redundant unwind rows (#2081)
8e9e24a 
As we binary search over the unwind information, any neighbouring rows
with the same unwind info can be summarised in just one.

This optimisation was obvious from early on and I added it back in
September last year (#794) but it was removed later
on and never re-added.

Test Plan
=========

Snapshot tests + added unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kakkoyun kakkoyun kakkoyun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@javierhonduco @kakkoyun