Install sane unwinding limit for SPS/breakpad

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

Trunk
mozilla23
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

As it stands, SPS/breakpad will unwind for up to 1024 frames (breakpad's
default limit), which just wastes a lot of CPU time for no purpose.  At
some point in the development process for bug 779291, there was a lower
limit installed, but that appears to have been lost in the complex
landing process for 779291.

This bug sets to a lower value (50 frames) which is seems very likely to
be more than enough for meaningful profiles, whilst keeping Breakpad from
falling into this particular performance hole.
Posted patch Patch (obsolete) — Splinter Review
In an ideal world, this could be done entirely on the SPS side
of the fence, with no changes to breakpad.  Unfortunately there
needs to be two-line change to breakpad to stop it spamming
us with messages in the now-common case where the unwind is
stopped by the limit.
Attachment #735110 - Flags: review?(ted)
50 will not be enough. The JS Engine is 'frame heavy'. When running in the interpreter each JS frame will yield something like 3 JS Engine frames on the C stack. I think 256 may be a better limit.
Comment on attachment 735110 [details] [diff] [review]
Patch

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

::: toolkit/crashreporter/google-breakpad/src/processor/stackwalker.cc
@@ +126,5 @@
>      stack->frames_.push_back(frame.release());
>      if (stack->frames_.size() > max_frames_) {
> +      // Don't spam the user with the following messages in the case
> +      // where a small unwind limit (100 or less) has been set, 
> +      // possibly in order to support profiling.

This isn't really fantastic, especially in light of BenWa's suggestion that we want a 256 frame limit for the profiler.

I think I'd be better off with making this not error if *any* non-default value was set for max_frames_.
Attachment #735110 - Flags: review?(ted) → review-
Posted patch Patch, v2Splinter Review
Addresses review comments:
* error message is disabled by setting the limit to anything
  (even the default, 1024)
* Changed requested unwind limit to 256 per BenWa comment
Attachment #735110 - Attachment is obsolete: true
Attachment #737485 - Flags: review?(ted)
Attachment #737485 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/b2d2b365e2e5
Assignee: nobody → jseward
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.