Closed Bug 859745 Opened 11 years ago Closed 11 years ago

Install sane unwinding limit for SPS/breakpad


(Core :: Gecko Profiler, defect)

Not set





(Reporter: jseward, Assigned: jseward)



(1 file, 1 obsolete file)

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.
Attached 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-
Attached 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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.