Closed
Bug 799818
Opened 12 years ago
Closed 12 years ago
IonMonkey: “getelem & MBoundsCheck” and “callprop & unbox” bailouts in pdf.js (octane)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: nbp, Assigned: nbp)
References
(Blocks 1 open bug)
Details
(Whiteboard: [ion:t])
Attachments
(3 files)
2.15 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
30.31 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
9.03 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Here are frequent subsets of the spew obtained while running the benchmark pdfjs benchmark provided in octane. [Bailouts] Bailing out pdfjs.js:17068, IonScript 0x546a770 [Bailouts] reading from snapshot offset 92 size 824 [Bailouts] Current script use count is 10241 [Bailouts] restoring frame [Bailouts] bailing from bytecode: getelem, MIR: boundscheck [16], LIR: boundscheck [13] [Bailouts] expr stack slots 2, is function frame 1 [Bailouts] frame slots 4, nargs 0, nfixed 2 [Bailouts] pushing 0 expression stack slots [Bailouts] new PC is offset 0 within script 0x7f45050aadd8 (line 17069) [Bailouts] Bounds check failure pdfjs.js:17068 [Bailouts] Took bailout! Snapshot offset: 346 [Bailouts] Bailing out pdfjs.js:26758, IonScript 0x4b0e2c0 [Bailouts] reading from snapshot offset 346 size 3992 [Bailouts] Current script use count is 12415 [Bailouts] restoring frame [Bailouts] bailing from bytecode: callprop, MIR: unbox [66], LIR: unbox [98] [Bailouts] expr stack slots 2, is function frame 1 [Bailouts] frame slots 6, nargs 0, nfixed 4 [Bailouts] pushing 0 expression stack slots [Bailouts] new PC is offset 129 within script 0x7f45050c9bb0 (line 26771) So after fixing these, it might be interesting to register a bailout counter per-script such as we can force a script to avoid compiling in Ion and fallback to JM if the use count increment in a small factor of the bailout counter. And use this counter to produce warnings which can potentially be detected by fuzzers.
Assignee | ||
Updated•12 years ago
|
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
It's a good idea to permanently deoptimize a function after a certain number of bailouts. I'm not sure I understand the fuzzer part. Is that to detect cases where we deoptimize the same access site over and over?
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to David Anderson [:dvander] from comment #1) > It's a good idea to permanently deoptimize a function after a certain number > of bailouts. I'm not sure I understand the fuzzer part. Is that to detect > cases where we deoptimize the same access site over and over? Yes, such as we can react to such de-optimization. The idea is that one bailout is fine, but hundreds of them from the same location without invalidation/deactivation is a performance issue.
It sounds like a deopt limit (bug 784134) should solve that.
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to David Anderson [:dvander] from comment #3) > It sounds like a deopt limit (bug 784134) should solve that. In such case, I don't think we are recompiling because the Ion compilation cost is (extremely) low in the callgrind profile.
Comment 5•12 years ago
|
||
If a GETELEM bounds check fails for the first time, we should set script->failedBoundsCheck and trigger a recompile. The next time we don't optimize any bounds checks. If the GETELEM has read |undefined|, it should start using GetElementHole. Why is that not happening here? Is it not an array?
Updated•12 years ago
|
Whiteboard: [ion:t]
Assignee | ||
Comment 6•12 years ago
|
||
Before this patch, if the bound check was hit, we were re-entering the same compiled function. When str[idx] is executed with an index which is not in the range [ 0 .. str.length [, then str[idx] will return undefined. So if type inference tell us that the return type is not guaranteed to be a string, we skip this optimization to prevent frequent bailouts.
Attachment #672095 -
Flags: review?(jdemooij)
Assignee | ||
Comment 7•12 years ago
|
||
When an element of an Array which does not have a well-known type is used in the expression of a switch, we will might get a double, but the Type inference will just report the propagated type, which is a MIRType_Value. This patch factor TableSwitch Lowering and CodeGenerator when possible and provides an extra specialization of TableSwitch MIRType_Value inputs.
Attachment #672096 -
Flags: review?(kvijayan)
Assignee | ||
Comment 8•12 years ago
|
||
When we have a singleton output, the input can be (as inferred by TI) a NULL or a String. In such case, this value might be interpreted as an unknown type which will produce a guard which ensure that an Object is flowing in and bail otherwise. This patch make sure that we distinguish the case where we have a String object as input and that we do not constantly bailout while hitting this guard.
Attachment #672099 -
Flags: review?(jdemooij)
Assignee | ||
Comment 9•12 years ago
|
||
Just as a note, the 3 previous patches are improving octane PdfJS benchmark by 6-7%, just by removing repeated bailouts. These patches do not seems to change much of the performances on V8, and I will need to run kraken and sunspider longer to make sure they do not regress anything.
Assignee | ||
Comment 10•12 years ago
|
||
Comment on attachment 672096 [details] [diff] [review] Handle unknown double as input of a table switch. Review of attachment 672096 [details] [diff] [review]: ----------------------------------------------------------------- Comment my own patch to details fixes needed to make it build & work on ARM. ::: js/src/ion/arm/Lowering-arm.cpp @@ +281,5 @@ > return defineReuseInput(lir, ins, 0); > } > > +LTableSwitch * > +LIRGeneratorX86Shared::newLTableSwitch(const LAllocation &in, const LDefinition &inputCopy, These are fixed (X86Shared -> ARM) in a local patch. @@ +297,2 @@ > bool > LIRGeneratorARM::visitTableSwitch(MTableSwitch *tableswitch) And this function is removed in a local patch. (as it already exists in ion/Lowering.cpp)
Comment 11•12 years ago
|
||
Comment on attachment 672095 [details] [diff] [review] Ensure return type before optimizing getelem for strings. Review of attachment 672095 [details] [diff] [review]: ----------------------------------------------------------------- Good catch, r=me with nits addressed. ::: js/src/ion/IonBuilder.cpp @@ +5037,5 @@ > current->add(length); > > + // This will cause an invalidation of this script once the 'undefined' type > + // is monitored by the interpreter. > + JS_ASSERT(oracle->propertyRead(script_, pc)->getKnownTypeTag() != JSVAL_TYPE_STRING); This should be == right? ::: js/src/ion/TypeOracle.cpp @@ +360,5 @@ > + // This function is used for jsop_getelem_string which should return > + // undefined if this is out-side the string bounds. Currently we just > + // fallback to a CallGetElement. > + StackTypeSet *pushed = script->analysis()->pushedTypes(pc, 0); > + if (pushed->baseFlags() & ~TYPE_FLAG_STRING) if (pushed->getKnownTypeTag() != JSVAL_TYPE_STRING)
Attachment #672095 -
Flags: review?(jdemooij) → review+
Comment 12•12 years ago
|
||
Comment on attachment 672099 [details] [diff] [review] Guard for strings when inlining known constants. Review of attachment 672099 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonBuilder.cpp @@ +4440,5 @@ > case JSVAL_TYPE_OBJECT: > case JSVAL_TYPE_UNKNOWN: { > + if (types->hasType(types::Type::StringType())) { > + // Do not optimize if the object is either a String or an Object. > + if (types->hasType(types::Type::AnyObjectType())) It can either have a list of TypeObjects or have the AnyObjectType. Use the following to check the former too: if (types->maybeObject())
Attachment #672099 -
Flags: review?(jdemooij) → review+
Comment 13•12 years ago
|
||
Comment on attachment 672096 [details] [diff] [review] Handle unknown double as input of a table switch. Review of attachment 672096 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/arm/Lowering-arm.cpp @@ +306,5 @@ > if (tableswitch->numSuccessors() == 1) > return add(new LGoto(tableswitch->getDefault())); > > + // If we don't know the type. > + if (opd->type() != MIRType_Value) { Shouldn't this be opd->type() == MIRType_Value ???
Comment 14•12 years ago
|
||
Comment on attachment 672096 [details] [diff] [review] Handle unknown double as input of a table switch. Review of attachment 672096 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/shared/CodeGenerator-x86-shared.h @@ +79,5 @@ > void emitBranch(Assembler::Condition cond, MBasicBlock *ifTrue, MBasicBlock *ifFalse, > NaNCond ifNaN = NaN_Unexpected); > void emitBranch(Assembler::DoubleCondition cond, MBasicBlock *ifTrue, MBasicBlock *ifFalse); > > + bool tableSwitchDispatch(MTableSwitch *mir, const Register &index, const Register &base); What about calling it emitTableSwitch()? All functions in CodeGenerator are visit* or emit* ... (sry, I know you didn't request my review, but I keep a closer eye on stuff I added. Feel free to ignore my comments)
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to Hannes Verschore from comment #13) > ::: js/src/ion/arm/Lowering-arm.cpp > @@ +306,5 @@ > > if (tableswitch->numSuccessors() == 1) > > return add(new LGoto(tableswitch->getDefault())); > > > > + // If we don't know the type. > > + if (opd->type() != MIRType_Value) { > > Shouldn't this be opd->type() == MIRType_Value ??? True, but this code has been removed, and this was a typo during the dev cycle. (In reply to Hannes Verschore from comment #14) > ::: js/src/ion/shared/CodeGenerator-x86-shared.h > > + bool tableSwitchDispatch(MTableSwitch *mir, const Register &index, const Register &base); > > What about calling it emitTableSwitch()? All functions in CodeGenerator are > visit* or emit* ... > > (sry, I know you didn't request my review, but I keep a closer eye on stuff > I added. Feel free to ignore my comments) Good idea, I will rename it.
Comment 16•12 years ago
|
||
Comment on attachment 672096 [details] [diff] [review] Handle unknown double as input of a table switch. Review of attachment 672096 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/arm/CodeGenerator-arm.h @@ +64,5 @@ > // true, and the false block if |cond| is false. > void emitBranch(Assembler::Condition cond, MBasicBlock *ifTrue, MBasicBlock *ifFalse); > > + bool tableSwitchDispatch(MTableSwitch *mir, const Register &index, const Register &base); > + Same comment from CodeGenerator-x86-shared.h - consider using "emitTableSwitchDispatch". ::: js/src/ion/arm/LIR-arm.h @@ +223,5 @@ > + MTableSwitch *mir() const { > + return mir_->toTableSwitch(); > + } > + > + static const size_t Index = 0; This would be clearer if it was named "InputValue" instead of "Index". The value being addressed by this position is the input. It might be used as an index, but that's only after some transforms occur on it (i.e. subtracting low(), scaling). At this point, it's clearer to refer to it as the input value than the index. ::: js/src/ion/shared/CodeGenerator-shared-inl.h @@ +41,5 @@ > return ToRegister(*def->output()); > } > > +static inline Register > +ToOptRegister(const LAllocation *a) An easier to grok name for this would be nice. |ToRegisterOrInvalid| or |ToRegisterMaybe| or something like that? ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp @@ +30,2 @@ > { } > Super nit: Extra spaces on the line after "{ }". Not introduced by you, but might as well clean it up when we see it :) ::: js/src/ion/shared/CodeGenerator-x86-shared.h @@ +79,5 @@ > void emitBranch(Assembler::Condition cond, MBasicBlock *ifTrue, MBasicBlock *ifFalse, > NaNCond ifNaN = NaN_Unexpected); > void emitBranch(Assembler::DoubleCondition cond, MBasicBlock *ifTrue, MBasicBlock *ifFalse); > > + bool tableSwitchDispatch(MTableSwitch *mir, const Register &index, const Register &base); I'd echo Hannes' comment here, but suggest "emitTableSwitchDispatch" instead, as that seems to describe well what the method does. ::: js/src/ion/shared/LIR-x86-shared.h @@ +135,5 @@ > + MTableSwitch *mir() const { > + return mir_->toTableSwitch(); > + } > + > + static const size_t Index = 0; Same as comment from LIR-arm.h: "InputValue" instead of "Index".
Attachment #672096 -
Flags: review?(kvijayan) → review+
Assignee | ||
Comment 17•12 years ago
|
||
dev branch testing, before applying nits: (JS test suite checked locally after nits) https://tbpl.mozilla.org/?tree=Try&rev=97f314659d45 https://tbpl.mozilla.org/?tree=Try&rev=9e51be3653be https://hg.mozilla.org/integration/mozilla-inbound/rev/9011919fcf00 https://hg.mozilla.org/integration/mozilla-inbound/rev/31ab0fe92304 https://hg.mozilla.org/integration/mozilla-inbound/rev/0498e3bb74bd
Comment 18•12 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/441defeeb653 - everything but b2g (and probably Android no-ion, which hasn't finished yet) burned. Randomly chosen log: https://tbpl.mozilla.org/php/getParsedLog.php?id=16279359&tree=Mozilla-Inbound
Assignee | ||
Comment 19•12 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #18) > Backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/441defeeb653 - > everything but b2g (and probably Android no-ion, which hasn't finished yet) > burned. Randomly chosen log: > https://tbpl.mozilla.org/php/getParsedLog.php?id=16279359&tree=Mozilla- > Inbound Ok, it seems that I checked locally the third patch without applying the last version of the second patch which contains a typo (forgot to rename all instances). Sorry. https://hg.mozilla.org/integration/mozilla-inbound/rev/47b425f4f50c https://hg.mozilla.org/integration/mozilla-inbound/rev/e70b2e6a9207 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4c7538d6a9
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/47b425f4f50c https://hg.mozilla.org/mozilla-central/rev/e70b2e6a9207 https://hg.mozilla.org/mozilla-central/rev/9e4c7538d6a9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #20) > Added in-testsuite? These are perf issues, the only mean I know for now to highlight these issues are using time-based testing, which I think is not desirable. Bug 803710 should provide a nice solution for that, and test should be added with up-coming regressions. In the mean time, these issues are part PdfJS which is provided with the octane benchmark and we might be able to diagnose it fast.
Updated•12 years ago
|
Flags: in-testsuite? → in-testsuite-
Updated•12 years ago
|
status-firefox19:
affected → ---
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•