859745 - Install sane unwinding limit for SPS/breakpad

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Julian Seward [:jseward]]

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

[Julian Seward [:jseward]]

Attachment: 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.

Comment 2

[Benoit Girard (:BenWa)]

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 3

[(not currently active) Ted Mielczarek]

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_.

Comment 4

[Julian Seward [:jseward]]

Attachment: 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

Comment 5

[(not currently active) Ted Mielczarek]

Landed the Breakpad bit upstream:
http://code.google.com/p/google-breakpad/source/detail?r=1150

Comment 6

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d2b365e2e5

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b2d2b365e2e5