862926 - Parallel JS: Generalize the DEBUG spew in ParCallToUncompiledScript.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

The old code assumed that the argument `func` will always have a
script associated with it.  But this is not true in some cases,
e.g. in the case of ES6 bound functions `(a,b) => ...`.
    
We must remove the assumption that the input function has a script.
We should also try to provide what information we can e.g. for bound functions.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v1: generalizes DEBUG spew on call to uncompiled

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Some relevant conversation from #jsapi:

  http://logbot.glob.com.au/?c=mozilla%23jsapi#c162408

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 738603 [details] [diff] [review]
patch A v1: generalizes DEBUG spew on call to uncompiled

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

Looks good. Just some style nits for our coding style.

::: js/src/ion/ParallelFunctions.cpp
@@ +226,5 @@
>      JS_ASSERT(InParallelSection());
>  
>  #ifdef DEBUG
> +    if (func->hasScript()) {
> +        RawScript script = func->nonLazyScript();

Nit: Suggested style going forward by the GC folks is that T * should be used over RawT for honesty. Some more instances below.

@@ +230,5 @@
> +        RawScript script = func->nonLazyScript();
> +        Spew(SpewBailouts, "Call to uncompiled script: %p:%s:%d",
> +             script, script->filename(), script->lineno);
> +    } else if (func->isBoundFunction()) {
> +        int maxDepth = 5;

Nit: Make this a static const and move to an easier-to-modify location.

@@ +236,5 @@
> +        JSFunction *target = func->getBoundFunctionTarget()->toFunction();
> +        while (depth < maxDepth) {
> +            if (target->hasScript()) {
> +                break;
> +            } else if (target->isBoundFunction()) {

Nit: No else if the then body is a jump.

@@ +238,5 @@
> +            if (target->hasScript()) {
> +                break;
> +            } else if (target->isBoundFunction()) {
> +                target = target->getBoundFunctionTarget()->toFunction();
> +            }

Nit: No surrounding braces if the body of a block is one line.

@@ +249,5 @@
> +        } else {
> +            Spew(SpewBailouts, "Call to bound function (excessive depth: %d)", depth);
> +        }
> +    } else {
> +        Spew(SpewBailouts, "Call to strange function.");

Should this be an assert? What are some legitimate functions that we can attempt to call in parallel that do not have a script and are not ES6 bound functions?

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v2: generalizes DEBUG spew on call to uncompiled

incoporating review comments from shu.

Carrying forward R+ from shu from earlier.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

try build: https://tbpl.mozilla.org/?tree=Try&rev=02e923227678

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dff432b7521

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9dff432b7521