Closed Bug 799818 Opened 7 years ago Closed 7 years ago

IonMonkey: “getelem & MBoundsCheck” and “callprop & unbox” bailouts in pdf.js (octane)

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ion:t])

Attachments

(3 files)

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: 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?
(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.
(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.
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?
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)
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)
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)
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.
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 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 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 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 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)
(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 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+
Blocks: 803710
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
(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
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: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(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.
Flags: in-testsuite? → in-testsuite-
Blocks: 804064
No longer blocks: 804064
Depends on: 804064
You need to log in before you can comment on or make changes to this bug.