bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Install sane unwinding limit for SPS/breakpad

RESOLVED FIXED in mozilla23

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
5 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)

(Assignee)

Description

5 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.
(Assignee)

Comment 1

5 years ago
Created attachment 735110 [details] [diff] [review]
Patch

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-
(Assignee)

Comment 4

5 years ago
Created attachment 737485 [details] [diff] [review]
Patch, v2

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