883126 - Improve performance of EXIDX unwinding in Breakpad

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Julian Seward [:jseward]]

(Our) Breakpad supports EXIDX unwinding for SPS, as of bug 863475.

However, performance is poor, due to a naive translation from the
EXIDX format into Breakpad's CFI representation.  Things we could
improve:

* constant fold the long "+ 4 + 4 + 4" postfix expressions

* (better) avoid postfix entirely, and generate non-postfix, fixed rules
  where possible

Also the space use is prohibitively large for use on FxOS.  Loading EXIDX
for libxul.so increases RSS by about 110MB.  Both of the above would
probably help.  If we stick with postfix (the more feeble solution)
then also we can common up the expression strings, so there's only
one copy of each.

Really it needs both time and space profiling.  Space profiling I can do
with V/Massif on Android, but meaningful time profiling is more difficult.

Comment 1

[(not currently active) Ted Mielczarek]

I suggested this in my final review, it's something I never liked about my original implementation. I think we ought to be able to get away with treating VSP as a module::Expr, since 99% of the time it's just an offset from sp.

Comment 2

[Julian Seward [:jseward]]

Attachment: Patch, v1

> I think we ought to be able to get away with treating VSP as a module::Expr

Patch that does exactly that.  It makes the presentation in 
arm_ex_to_module.cc a bit clearer, too.  It adds two new methods to
Module::Expr, to add an offset to an Expr and to create a dereferenced
version of an Expr.

Performance improvement is difficult to quantify, but is at least a
factor of 3 and possibly much more.  With it in place, I can do
EXIDX unwinding at 500Hz on a dual-core A9 without losing samples.
Memory consumption for loading libxul.so EXIDX is reduced by at
least 10MB.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 765359 [details] [diff] [review]
Patch, v1

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

Pretty much just nit-picking, great to hear that this fix results in a sizeable speedup! Do you have any idea how often vsp gets downgraded to a postfix expression in practice?

Also, I think we seriously need to add some Module::Expr unit tests in module_unittest.cc, but I won't block you on that (since we don't run those for Firefox builds anyway).

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_to_module.cc
@@ +96,5 @@
>          }
>        }
>        break;
>      case ARM_EXIDX_CMD_SUB_FROM_VSP:
> +      vsp = vsp.add_delta(- (long)edata->data);

Google style frowns upon non-C++ casts.

@@ +105,5 @@
>      case ARM_EXIDX_CMD_REG_POP:
>        for (unsigned int i = 0; i < 16; i++) {
>          if (edata->data & (1 << i)) {
> +          entry->initial_rules[ToUniqueString(regnames[i])] = vsp.deref();
> +          vsp = vsp.add_delta(4);

These changes result in a nice net cleanup in this file!

@@ +147,5 @@
>      case ARM_EXIDX_CMD_WCGR_POP:
>        // Pop wCGR registers under mask {wCGR3,2,1,0}, hence "i < 4"
>        for (unsigned int i = 0; i < 4; i++) {
>          if (edata->data & (1 << i)) {
> +          vsp = vsp.add_delta(4);

Reading all these I wonder if add_delta ought to just modify the Expr in-place instead of returning a new one? That would presumably simplify things further.

@@ +164,5 @@
>    stack_frame_entry_ = new Module::StackFrameEntry;
>    stack_frame_entry_->address = addr;
>    stack_frame_entry_->size = size;
> +  Module::Expr sp_expr = Module::Expr(ustr__sp(), 0, false); // "sp"
> +  stack_frame_entry_->initial_rules[ToUniqueString(kCFA)] = sp_expr;

Only just noticed this, but this can just be ustr__ZDcfa(), right? (similarly for kRA below). Probably just fallout from this patch being written pre-UniqueString.

::: toolkit/crashreporter/google-breakpad/src/common/module.h
@@ +176,5 @@
> +          return postfix_;
> +        case kExprSimple:
> +        case kExprSimpleMem: {
> +          char buf[40];
> +          sprintf(buf, " %ld %c%s", offset_ < 0 ? -offset_ : offset_,

Just use abs(offset_) here.

@@ +183,5 @@
> +          return string(FromUniqueString(ident_)) + string(buf);
> +        }
> +        case kExprInvalid:
> +        default:
> +          return "Expr::genExprPostfix: kExprInvalid";

Not really wild about this. Can you at least throw in an assert?

@@ +196,5 @@
>      }
>  
> +    // Returns an Expr which evaluates to |this| + |delta|
> +    Expr add_delta(long delta)
> +    {

nit: opening brace on the same line as the function name.

@@ +207,5 @@
> +      //    *(identifier + offset)
> +      //    completely arbitrary postfix expression string
> +      // we'll need to "downgrade" it to a postfix expression and stick
> +      // "+ delta" at the end of the string, since we can't represent
> +      // the result in the simple form.

nit: use third-person in comments. Any idea how often we hit the non-trivial case in practice?

@@ +212,5 @@
> +      switch (how_) {
> +        case kExprSimpleMem:
> +        case kExprPostfix: {
> +          char buf[40];
> +          sprintf(buf, " %ld %c", delta < 0 ? -delta : delta,

abs, as above.

@@ +220,5 @@
> +        case kExprSimple:
> +          return Expr(ident_, offset_ + delta, false);
> +        case kExprInvalid:
> +        default:
> +          // Invalid inputs produce an invalid result

as above, I would like an assert here.

@@ +227,5 @@
> +    }
> +
> +    // Returns an Expr which evaluates to *|this|
> +    Expr deref()
> +    {

nit: opening bracket on previous line

@@ +230,5 @@
> +    Expr deref()
> +    {
> +      // In the simplest case, we can change a kExprSimple into a
> +      // kExprSimpleMem.  In all other cases we have to dump it as a
> +      // postfix string and add " ^" at the end.

nit: third-person in comments

@@ +243,5 @@
> +          return Expr(getExprPostfix() + " ^");
> +        }
> +        case kExprInvalid:
> +        default:
> +          // Invalid inputs produce an invalid result

assert, as above.

Comment 4

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69bb6b4fd171

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69bb6b4fd171