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.

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

Improve DsoHdr::erase_range #370

Merged
merged 7 commits into from
Jan 26, 2024
Merged

Improve DsoHdr::erase_range #370

merged 7 commits into from
Jan 26, 2024

Conversation

nsavoire
Copy link
Collaborator

@nsavoire nsavoire commented Jan 25, 2024

  • Add DDPROF_CHECK_FATAL / DDPROF_DCHECK_FATAL assertions
  • Add function to check DsoHdr invariants
  • Change pid_fork to do nothing if child pid mappings already exist
    pid_fork directly copies mappings from parent to child pid,
    if child pid already has mappoings this can lead to overlapping mappings.
    Currently this cannot happens, because child mappings are cleared before
    pid_fork. This check adds a safegard by returning early if child pid
    has mappings.
  • Add DsoHdr invariant check in debug mode after inserting a Dso
  • Make erase_range function cleaner
    • Add check when re-inserting truncated last mapping
    • Return early in the case new mapping truncates end of a single mapping
      to avoid using range.first ierator that is invalidated by map::extract
      (the node pointed by first is extracted and then re-inserted hence
      the iterator should remain valid, but better be careful)
  • Allow anonymous mapping to truncate existing file mapping
    Allow new mapping to truncate existing mappings if previous mapping is
    a file and new mapping is anonymous because a LOAD segment can have
    filesize < memsize as a size optimization when the end of segment is all
    zeros (eg. bss). In that case elf loader / dlopen will map the whole
    file + some extra space to cover the difference between memsize and file
    size and then map an anonymous mapping over the part not in the file.
  • Avoid out-of-bound array access when printing unwinding error in debug logs

@nsavoire nsavoire requested a review from r1viollet January 25, 2024 16:18
@nsavoire nsavoire force-pushed the nsavoire/clean_erase_range branch 2 times, most recently from a9d1ecd to a704bfb Compare January 26, 2024 08:48
`pid_fork` directly copies mappings from parent to child pid,
if child pid already has mappoings this can lead to overlapping mappings.
Currently this cannot happens, because child mappings are cleared before
`pid_fork`. This check adds a safegard by returning early if child pid
has mappings.
* Add check when re-inserting truncated last mapping
* Return early in the case new mapping truncates end of a single mapping
  to avoid using range.first ierator that is invalidated by map::extract
  (the node pointed by first is extracted and then re-inserted hence
   the iterator should remain valid, but better be careful)
@nsavoire nsavoire force-pushed the nsavoire/clean_erase_range branch from a704bfb to c6d5a4f Compare January 26, 2024 10:10
@nsavoire nsavoire changed the title Make erase_range cleaner Improve DsoHdr::erase_range Jan 26, 2024
Allow new mapping to truncate existing mappings if previous mapping is
a file and new mapping is anonymous because a LOAD segment can have
filesize < memsize as a size optimization when the end of segment is all
zeros (eg. bss). In that case elf loader / dlopen will map the whole
file + some extra space to cover the difference between memsize and file
size and then map an anonymous mapping over the part not in the file.
@nsavoire nsavoire force-pushed the nsavoire/clean_erase_range branch from c6d5a4f to 284e23b Compare January 26, 2024 10:22
@nsavoire nsavoire marked this pull request as ready for review January 26, 2024 10:22
@nsavoire nsavoire requested a review from sanchda as a code owner January 26, 2024 10:22
@pr-commenter
Copy link

pr-commenter bot commented Jan 26, 2024

Benchmark results for collatz

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.15.3+4e33d7a4.27091101 ddprof 0.15.3+284e23be.27241456

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

See unchanged results
scenario Δ mean execution_time
scenario:ddprof -S bench-collatz --preset cpu_only collatz_runner.sh same

@pr-commenter
Copy link

pr-commenter bot commented Jan 26, 2024

Benchmark results for BadBoggleSolver_run

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.15.3+4e33d7a4.27091101 ddprof 0.15.3+284e23be.27241456

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

See unchanged results
scenario Δ mean execution_time
scenario:ddprof -S bench-bad-boggle-solver BadBoggleSolver_run work 1000 same

Copy link
Collaborator

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM
We had to do a dedicated session to review some of the changes here.
The erase_range logics are not easy to understand and require a careful understanding of linker behaviours.

@nsavoire nsavoire merged commit a078c6c into main Jan 26, 2024
2 checks passed
@nsavoire nsavoire deleted the nsavoire/clean_erase_range branch January 26, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants