Closed Bug 892774 Opened 7 years ago Closed 7 years ago

Breakpad Stack scan: speed up MyCodeModules::GetModuleForAddress

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: BenWa, Assigned: jseward)

References

Details

Attachments

(1 file)

According to this profile by jld:
https://people.mozilla.com/~bgirard/cleopatra/#report=2af1780356d123e3e2c91102441e2e48c0d95611&search=breakpad

We don't us/ report stack scanning currently and it's really hurting us (We might later).
Stack scanning is slow because the basic action is to take words off
the stack and see if they point just after call instructions.  To do
that safely (without segfaults) requires calling
MyCodeModules::GetModuleForAddress for each word.  The implementation
is stupid in that it searches sequentially though the known modules.
Patch to follow.
Attached patch Patch, v1Splinter Review
Patch that adds two levels of optimisation to MyCodeModules::GetModuleFromAddress:
* binary search for the module instead of sequential search
* cache min/max code addresses, so as to throw out obviously-out-of-range
  requests without binary search

Testing on x86_64-linux, this reduces the number of tested MyCodeModules
per call from about 65 to about 5, viz, a factor of more than 10 improvement.

I also discovered that around 55% of the calls to GetModuleFromAddress
find no module, presumably because the stack scanner is feeding it
values which are either small integers, stack addresses or heap addresses
(that it finds on the stack).

Patch is defensively written.  It carefully audits what it gets
from SharedLibraryInfo::GetInfoForSelf, so as to ensure that the binary
search cannot get into infinite loops even if GetInfoForSelf gives us
garbage results (out of order or overlapping address ranges).
Attachment #775321 - Flags: review?(bgirard)
Summary: Breakpad Stack scanning is slowing down unwinds → Breakpad Stack scan: speed up MyCodeModules::GetModuleForAddress
Assignee: nobody → jseward
Comment on attachment 775321 [details] [diff] [review]
Patch, v1

Review of attachment 775321 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/UnwinderThread2.cpp
@@ +1705,3 @@
>      }
> +
> +    // Binary search in |mods_|.  lo and hi need to be signed, else

You could use http://www.cplusplus.com/reference/algorithm/binary_search/ with the iterator of mods_.
Attachment #775321 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/8cc1cd14c1fd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.