The default bug view has changed. See this FAQ.

IonMonkey: huge regression on kraken-gaussian-blur due to uninitialized specialization field in MAbs

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 615881 [details] [diff] [review]
/home/mrosenberg/patches/abs_specialize-r0.patch

After I landed some extra range analysis code (bug 738873), kraken-gaussian-blur took a 27000% hit on perf!  The issue was a field added to MAdd, which changed the uninitialized values in MAbs::specialization_.  This lead to the abs being specialized incorrectly, and ultimately, us bailing out of the jit, and never returning.  I suspect that the lack of returning to the jit is as much of a problem as the bailing out in the first place.
Attachment #615881 - Flags: review?(nicolas.b.pierron)
Comment on attachment 615881 [details] [diff] [review]
/home/mrosenberg/patches/abs_specialize-r0.patch

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

oracle->unaryOp is unlikely to work as expected on a Call.  Use the MIRType extracted by the function to provide the information to the infer function.

Sorry for misleading you in that direction, I forgot that MAbs was not a bytecode.

::: js/src/ion/MCallOptimize.cpp
@@ +153,5 @@
>                  arg1Type == returnType) {
>                  if (!discardCall(argc, argv, current))
>                      return false;
>                  MAbs *ins = MAbs::New(argv[1], returnType);
> +                ins->infer(oracle->unaryOp(script, pc));

hum … I forgot that this was a call, I don't know if we can safely use unaryOp here because this is not a unary-operation of the Bytecode.

You can change the prototype of the function to get only the input type.

::: js/src/ion/MIR.cpp
@@ +673,5 @@
> +void
> +MAbs::infer(const TypeOracle::Unary &u)
> +{
> +    if (u.ival == MIRType_Object)
> +        specialization_ = MIRType_None;

To me, this case should never happen.  You should prefer JS_ASSERT in this case.

@@ +675,5 @@
> +{
> +    if (u.ival == MIRType_Object)
> +        specialization_ = MIRType_None;
> +    else if (u.ival == MIRType_Double)
> +        specialization_ = MIRType_Double;

specialization_ = u.ival;

and move it to the header file.

::: js/src/ion/MIR.h
@@ +2032,5 @@
>          JS_ASSERT(type == MIRType_Double || type == MIRType_Int32);
>          setResultType(type);
>      }
>  
> +

really ?
Attachment #615881 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 2

5 years ago
Created attachment 615951 [details] [diff] [review]
/home/mrosenberg/patches/abs_specialize_differencly-r0.patch

Different approach, just use the return value, since we ensure that it is the same as the argument; We can probably also handle the value case, as well as the Abs :: Int32 -> Value case.
Attachment #615881 - Attachment is obsolete: true
Attachment #615951 - Flags: review?(nicolas.b.pierron)
Attachment #615951 - Flags: review?(nicolas.b.pierron) → review+
We should have a follow-up bug for the valgrind instrumentation to detect these kinds of errors.
(Reporter)

Comment 4

5 years ago
landed: http://hg.mozilla.org/projects/ionmonkey/rev/746335
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 5

5 years ago
*cough* http://hg.mozilla.org/projects/ionmonkey/rev/8baf654d39c3
You need to log in before you can comment on or make changes to this bug.