Closed
Bug 746335
Opened 12 years ago
Closed 12 years ago
IonMonkey: huge regression on kraken-gaussian-blur due to uninitialized specialization field in MAbs
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
782 bytes,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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 1•12 years ago
|
||
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•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #615951 -
Flags: review?(nicolas.b.pierron) → review+
Comment 3•12 years ago
|
||
We should have a follow-up bug for the valgrind instrumentation to detect these kinds of errors.
Reporter | ||
Comment 4•12 years ago
|
||
landed: http://hg.mozilla.org/projects/ionmonkey/rev/746335
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 5•12 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.
Description
•