814177 - IonMonkey: InstanceOf codegen uses 2 consecutive calls.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

The live registers in a safepoint include those which are live after the instruction ends (excluding the outputs of the instruction).  HasInstance uses a GetProperty cache followed by a HasInstance test, where both involve an OOL VM call which uses the instruction's safepoint.  If inputs to the safepoint are in registers (which LSRA does not seem to do in this case, but other register allocators could), they are not required to be in the live registers, but if the GetPropertyCache call is then made it will not restore the input registers, and incorrect values can flow into the second HasInstance call and cause it to behave incorrectly.

Comment 1

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

A simple rule:
- One runtime call at most per codegen.

Failing to respect this rule might have plenty of bad consequences such as bad marking, bad invalidation or bad bailouts.

No ->rejoin() should appear before an ->entry(), Good catch.

Comment 2

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

This is not the same as Bug 814396 as CharCodeAt as only one call site.

The problem with multiple calls per codegen is that the backward patch made during the invalidation might *cause random code to be executed* in the rare case where one of the call is calling the same function and enter the other branch causing an invalidation.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

What plan of attack are you considering? I was thinking we'd have a specialized IC or decomposed opcodes (that might be tricky), but I think Brian also had a plan to use more type information.

Comment 4

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

My plan is to split it in to different codegen, such as we can get an OSIpoint in the middle.  The problem is that this solution is not a good solution as it might cause a double execution effect if we do not resume the second part of the instanceOf operation.

Another solution would be to produce an out-of-line path used in case of invalidation where a non-backward patched call can be used, but this sounds too hacky to me.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Ideally we just wouldn't have that GetPropertyCache sitting in the middle. Either the whole thing could be a specialized IC, or maybe we can inline more aggressively with good type information.

Comment 6

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Bug introduced while fixing Bug 794947, and later modified by Bug 803730 for fixing another issue on the same code.

(In reply to David Anderson [:dvander] from comment #5)
> Ideally we just wouldn't have that GetPropertyCache sitting in the middle.
> Either the whole thing could be a specialized IC, or maybe we can inline
> more aggressively with good type information.

I agree, after looking more at the code, the best would be to have an inline cache for it.  More type information does not solve the problem as we have to be able to compile without any type information to handle non-visited branches.

Comment 7

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

The faulty function CodeGenerator::emitInstanceOf implements fun_hasInstance (sadly not named in any comments near the function) and fallback to the VM function call if this is not a function.  Which means that the GetProperty IC which is used is constantly used with a function object.  The GetProperty is done to recover the prototype of the function object.  The second loop inline the function IsDelegate and fallback to the VM function call in case the getProto is made on a Proxy.

So,

1/ The path which test that we obtain an object at the end of the getProperty should raise an exception instead of jumping to the done label with a bad output. (see fun_hasInstance)

2/ Calling the VM function as a fallback in the IsDelegate assembly version is wrong because it cause a re-evaluation of the GetProperty.

I suggest to do 2 fixes:

1/ Make an IC which do a GetProperty (of .prototype) & IsDelegate, which means that the loop unrolling bind-links (which fallback on the ool call) can reduce the number of inputs of the cache. [fix this bug]

2/ Add an unconditional halt to the reverse patch mechanism to prevent random code execution (cause a controlled crash) and check that there is enough space since the last masm.bind to prevent erasing a path from a return value to the osi point. (assuming we don't generate dead code this should be enough) [prevent useful exploit of this kind of bug]

Comment 8

[Brian Hackett [Laid off!]]

Why not just make instanceof a VM call in cases where type information is bad?  I'm working right now on an optimized instanceof that bakes in knowledge of rhs-is-a-function and the rhs's .prototype (will file that in a separate bug).  Skimming, that should handle every instanceof in the benchmarks (basically just earley-boyer), and all the uses of instanceof in jQuery and Prototype.js except one which the existing GetPropertyCache mechanism probably doesn't handle well either.  Also, bug 717466 comment 3 suggests there is relatively little to be gained from the assembly version (so little static information is being baked in), though I don't know how different that is from what's in the tree.

Comment 9

[Brian Hackett [Laid off!]]

Bug 814861 has the type specialized paths for instanceof.  Also, given how awful v8 is at polymorphic instanceof (even with statically known rhs, see bug 814861), I think it should be fine if Ion just does a stub call if rhs is not statically known.

Comment 10

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Brian Hackett (:bhackett) from comment #8)
> Why not just make instanceof a VM call in cases where type information is
> bad?

Sounds like a good solution if we don't regress dromaeo or other benchmarks otherwise we would have to backport the type-specialized version too.

Comment 11

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Attachment: Use callVM to implement all cases of InstanceOf.

As bhackett mentionned, Bug 814861 is supposed to cover common patterns where the rhs can be assumed as a constant.  Still when the rhs has never been evaluated before, we have to use a generic instanceOf implementation.

This patch use a callVM to handle all cases of InstanceOf via the corresponding interpreter function.  This will cause a slow down in our IonMonkey implementation, but will provide a fault-free solution while waiting for Bug 814861.

If there is any performance slow down, we should think of backporting Bug 814861 patch, which does not have the issue reported in this bug because the getProperty is done while building the IonGraph.

Note: The test case for Bug 794947 should be modify to work with --no-jm, because this patch will be used by the eager compilation and would mute the test case for testing Bug 814861.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 684896 [details] [diff] [review]
Use callVM to implement all cases of InstanceOf.

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

Good to get rid of this code... r=me with some renamings now that bhackett's patch landed:
  MInstanceOf -> MCallInstanceOf
  LInstanceOfV -> LCallInstanceOf
  MInstanceOfTyped -> MInstanceOf
  LInstanceOfTyped -> LInstanceOf

Comment 13

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ed5864b6eb

Comment 14

[Hannes Verschore [:h4writer]]

FYI this causes a 3% regression in v8-raytrace

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d3ed5864b6eb

Comment 16

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 684896 [details] [diff] [review]
Use callVM to implement all cases of InstanceOf.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 794947

User impact if declined: 
Random code execution / crash

Testing completed (on m-c, etc.): 
see comment 15.

Risk to taking this patch (and alternatives if risky): 
Possible performance regressions with IonMonkey instanceof usage.  In such case we can backport Bug 814861 too which should handle most of the common use cases without having any similar issue as the previous implementation.

String or UUID changes made by this patch:
N/A

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 684896 [details] [diff] [review]
Use callVM to implement all cases of InstanceOf.

[Triage Comment]
To prevent a FF18 sec-critical regression.

Comment 18

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5038ad88dabe

Comment 19

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b8651fe7c5d

Comment 20

[Boris Zbarsky [:bzbarsky]]

On branches, this is a 5% hit or so on V8-v7 on Mac.  Is that expected?

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

Good catch, Boris. This patch should have gone in with or before bug 814861.

Comment 22

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Boris Zbarsky (:bz) from comment #20)
> On branches, this is a 5% hit or so on V8-v7 on Mac.  Is that expected?

Yes, as mentioned in comment 16.

> Risk to taking this patch (and alternatives if risky):
> Possible performance regressions with IonMonkey instanceof usage.  In such
> case we can backport Bug 814861 too which should handle most of the common
> use cases without having any similar issue as the previous implementation.

(In reply to David Anderson [:dvander] from Bug 814861 comment #9)
> Since bug 814177 regressed beta performance significantly, we should either
> back that out or take this on beta. I think this is low risk enough - what
> do you think, Brian?

Doing a backout of this patch on aurora / beta sounds like a bad solution without a substitute. As you & I are mentioning, Bug 814177 seems like a low risk which does not have any of the bug found on the previous InstanceOf implementation.

Comment 23

[(no longer active)]

This regressed v8 on Aurora.  Intentional?

Regression  V8 version 7 decrease 8.73% on Win7 Mozilla-Aurora
----------------------------------------------------------------
    Previous: avg 6477.200 stddev 116.582 of 30 runs up to revision bf1ceb185f0c
    New     : avg 5911.488 stddev 70.268 of 5 runs since revision 5038ad88dabe
    Change  : -565.712 (8.73% / z=4.852)
    Graph   : http://mzl.la/YOFzXT

Changeset range: http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=bf1ceb185f0c&tochange=5038ad88dabe

Changesets:
  * http://hg.mozilla.org/releases/mozilla-aurora/rev/0c2e87d10933
    : Sriram Ramasubramanian <[e-mail]> - Bug 786982: Add the hiding of close tab, while swiping. r=lucasr a=lsblakk
    : http://bugzilla.mozilla.org/show_bug.cgi?id=786982

  * http://hg.mozilla.org/releases/mozilla-aurora/rev/5038ad88dabe
    : Nicolas B. Pierron <[e-mail]> - Bug 814177 - Use a callVM for generic InstanceOf cases. r=dvander a=akeybl
    : http://bugzilla.mozilla.org/show_bug.cgi?id=814177

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=814177
  * http://bugzilla.mozilla.org/show_bug.cgi?id=786982 - The user should be able to close the last remaining tab
_______________________________________________
dev-tree-management mailing list
[e-mail]
https://lists.mozilla.org/listinfo/dev-tree-management

Comment 24

[(no longer active)]

(Same for beta)



Regression  V8 version 7 decrease 4.27% on MacOSX 10.7 Mozilla-Beta
---------------------------------------------------------------------
    Previous: avg 5573.204 stddev 65.916 of 30 runs up to revision 7d4da1832dcc
    New     : avg 5334.976 stddev 31.586 of 5 runs since revision 6b8651fe7c5d
    Change  : -238.228 (4.27% / z=3.614)
    Graph   : http://mzl.la/RADufY

Changeset range: http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=7d4da1832dcc&tochange=6b8651fe7c5d

Changesets:
  * http://hg.mozilla.org/releases/mozilla-beta/rev/6b8651fe7c5d
    : Nicolas B. Pierron <[e-mail]> - Bug 814177 - Use a callVM for generic InstanceOf cases. r=dvander a=akeybl
    : http://bugzilla.mozilla.org/show_bug.cgi?id=814177

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=814177
_______________________________________________
dev-tree-management mailing list
[e-mail]
https://lists.mozilla.org/listinfo/dev-tree-management

Comment 25

[(no longer active)]

Sorry, didn't read the bug well enough!