Closed Bug 1003825 Opened 6 years ago Closed 6 years ago

Get funapply tests working again

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch get-funapply-test-working (obsolete) — Splinter Review
Just making sure the tests test again for which they were added. They didn't fail anymore when disabling the baselineBailouts hacks.
Assignee: nobody → hv1989
Attachment #8415283 - Flags: review?(jdemooij)
Attached patch get-funapply-test-working (obsolete) — Splinter Review
Forgot to qref
Attachment #8415283 - Attachment is obsolete: true
Attachment #8415283 - Flags: review?(jdemooij)
Attachment #8415285 - Flags: review?(jdemooij)
Comment on attachment 8415285 [details] [diff] [review]
get-funapply-test-working

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

::: js/src/jit/IonBuilder.cpp
@@ +4978,5 @@
> +    JSFunction *target = getSingleCallTarget(calleeTypes);
> +
> +    // Try optimizing fun.apply(foo, arguments).
> +    do {
> +        if (info().executionMode() != DefinitePropertiesAnalysis)

Should be ==, but then we'll no longer inline for the definite properties analysis, see this comment in jsop_funapplyarguments:

    // We also try this path when doing the definite properties analysis, as we
    // can inline the apply() target and don't care about the actual arguments
    // that were passed in.

@@ +4998,5 @@
> +        if (!target)
> +            break;
> +
> +        if (!target->isNative() || target->native() != js_fun_apply)
> +            break;

If we have:

    fun.apply(foo, arguments);

And fun.apply is not js_fun_apply, we'll now emit a call with MIRType_MagicOptimizedArguments as argument. js_fun_apply knows what to do with MagicValue(JS_OPTIMIZED_ARGUMENTS), but this will fail for other callees.
Attachment #8415285 - Flags: review?(jdemooij)
Attachment #8415285 - Attachment is obsolete: true
Attachment #8416488 - Flags: review?(jdemooij)
Depends on: 1005030
Comment on attachment 8416488 [details] [diff] [review]
Patch: without the funapply changes

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

::: js/src/jit/IonBuilder.cpp
@@ +642,5 @@
>          IonSpew(IonSpew_Scripts, "Recompiling script %s:%d (%p) (usecount=%d, level=%s)",
>                  script()->filename(), script()->lineno(), (void *)script(),
>                  (int)script()->getUseCount(), OptimizationLevelString(optimizationInfo().level()));
>      } else {
> +        IonSpew(IonSpew_Scripts, "Compiling script %s:%d (%p) (usecount=%d, level=%s)",

Nice to have this fixed.

::: js/src/jsinfer.h
@@ +156,5 @@
>       * when it is not known whether a lazy arguments value can be used.
>       */
>      ArgumentsUsageAnalysis
>  };
> +inline const char *

Nit: add an empty line before this line.
Attachment #8416488 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #5)
> ::: js/src/jsinfer.h
> @@ +156,5 @@
> >       * when it is not known whether a lazy arguments value can be used.
> >       */
> >      ArgumentsUsageAnalysis
> >  };
> > +inline const char *
> 
> Nit: add an empty line before this line.

Strangely this had already a newline in my original patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/079264ecb678
https://hg.mozilla.org/mozilla-central/rev/079264ecb678
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.