Install sane unwinding limit for SPS/breakpad

RESOLVED FIXED in mozilla23



6 years ago
6 years ago


(Reporter: jseward, Assigned: jseward)



Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



6 years ago
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.

Comment 1

6 years ago
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]

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

::: toolkit/crashreporter/google-breakpad/src/processor/
@@ +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-

Comment 4

6 years ago
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+
Assignee: nobody → jseward
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.