1326501 - Baldr: optimize getFuncName()

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Luke Wagner [:luke]]

It's a bit clownshoes right now (my own fault), going between UTF8->two-byte->UTF8 on various paths.  This patch drops the time to create all the profiling labels for all functions (when profiling is enabled) from ~80ms to 8-16ms.  It also removes a 'cx' arg that a dependent patch doesn't have.

Comment 1

[Luke Wagner [:luke]]

Attachment: optimize-get-func-name

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8822789 [details] [diff] [review]
optimize-get-func-name

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

It's nicely simpler, thanks for the patch!

::: js/src/wasm/WasmCode.cpp
@@ +549,5 @@
>  
>          if (n.length == 0)
>              goto invalid;
>  
> +        return name->append((const char*)maybeBytecode->begin() + n.offset, n.length);

Now that the control flow has been simplified, how about:

if (n.length != 0)
    return name->append(...);

then the "invalid" label can be removed and we won't need a goto at all.

@@ +766,5 @@
>      // do it now since, once we start sampling, we'll be in a signal-handing
>      // context where we cannot malloc.
>      if (newProfilingEnabled) {
> +        if (!funcLabels_.resize(metadata_->codeRanges.length()))
> +            return false;

Not sure to understand this change: the final funcLabels_.length() *before* < metadata_->codeRanges.length() because of entries, exits, inline code, etc. So this is pretty pessimistic, and I think makes the `if (codeRange.funcIndex() >= funcLabels_.length())` body (in the loop below) unreachable.

Or maybe we could keep this, remove the `if` in the for loop's body, and after the loop, resize funcLabels_ to fit its real length?

Comment 3

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> > +        if (!funcLabels_.resize(metadata_->codeRanges.length()))
> > +            return false;
> 
> Not sure to understand this change:

Oops, this was a remnant from an earlier attempt to simplify by resizing up front instead of growing on demand (just as you suggested for the reason you mentioned).  This wouldn't have been too pessimistic since most code labels are for functions.  However, this broke on asm.js where we have a func-index hole (so there are *fewer* CodeRanges than the max func-index).  I forgot to remove this line when undoing the change though so good catch!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d8943da05fb
Baldr: optimize getFuncName (r=bbouvier)

Comment 5

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/1d8943da05fb