Last Comment Bug 746335 - IonMonkey: huge regression on kraken-gaussian-blur due to uninitialized specialization field in MAbs
: IonMonkey: huge regression on kraken-gaussian-blur due to uninitialized speci...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 738873
  Show dependency treegraph
 
Reported: 2012-04-17 14:35 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2012-07-27 18:58 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/abs_specialize-r0.patch (2.62 KB, patch)
2012-04-17 14:35 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
/home/mrosenberg/patches/abs_specialize_differencly-r0.patch (782 bytes, patch)
2012-04-17 16:50 PDT, Marty Rosenberg [:mjrosenb]
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-04-17 14:35:08 PDT
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.
Comment 1 Nicolas B. Pierron [:nbp] 2012-04-17 15:02:31 PDT
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 ?
Comment 2 Marty Rosenberg [:mjrosenb] 2012-04-17 16:50:10 PDT
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.
Comment 3 Sean Stangl [:sstangl] 2012-04-17 17:16:29 PDT
We should have a follow-up bug for the valgrind instrumentation to detect these kinds of errors.
Comment 4 Marty Rosenberg [:mjrosenb] 2012-07-27 18:57:32 PDT
landed: http://hg.mozilla.org/projects/ionmonkey/rev/746335
Comment 5 Marty Rosenberg [:mjrosenb] 2012-07-27 18:58:20 PDT
*cough* http://hg.mozilla.org/projects/ionmonkey/rev/8baf654d39c3

Note You need to log in before you can comment on or make changes to this bug.