807391 - IonMonkey: mjit caches could erroneous report being single shaped

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Attachment: Possible fix

Reference: v8-crypto:431

If a a property get/set is always single shaped, except once in 10000+ times it will make us bail every time. This is because mjit caches will report the property being single shaped, because it only observed it to be single shaped. 

Crypto has this problem.

Comment 1

[Hannes Verschore [:h4writer]]

I already found out this is not exactly the fix we want, because it regresses JSNES enormous. But I have an idea how to fix it.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Possible fix 2

- Don't look at mjit to decide when to inline property access
- Adjust MShapeGuard to test all shapes between obj->lastProperty() to requested shape, before bailing.

JSNes: doesn't regress
V8-crypto: all bailouts are gone

@Brain: this is the easy patch. I have a more complicated patch that transformed the MShapeGuard into a cache that had 4 slots to keep guarded shapes and a ool path to update the slots or to bail. Overall it had an improvement on V8, but v8-raytrace regressed really bad!

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> - Don't look at mjit to decide when to inline property access
I ment: property setting (getting is still inlined)

Comment 4

[Hannes Verschore [:h4writer]]

New information. 

We bail in crypto.js:431 because the shapeguards report only "r.array" exists. But the object has actually also "r.s" and "r.t". Those are always added in crypto.js:180, bnpCopyTo. In IM we specialized to "setpropertycachet" to add those. I assume setPropertyCache doesn't update the shape of the object accordingly...

Stacktrace of 432
bnpSquareTo(r=BigInteger { array=[0], copyTo=bnpCopyTo(), fromInt=bnpFromInt(), more...})crypto.js (line 432)
montSqrTo(x=BigInteger { array=[37], t=37, s=0}, r=BigInteger { array=[0], more...})crypto.js (line 603)
bnpExp(e=65537, z=Montgomery { m=BigInteger, mp=120750245, mpl=165, more...})crypto.js (line 626)
bnModPowInt(e=65537, m=BigInteger { array=[37], t=37, s=0})crypto.js (line 637)
RSADoPublic(x=BigInteger { array=[37], t=37, s=0})crypto.js (line 1522)
RSAEncrypt(text="The quick brown fox jum...n to come to the party.")crypto.js (line 1529)
encrypt()crypto.js (line 1687)
RunSingleBenchmark(...)base.js (line 205)
RunNextBenchmark()base.js (line 244)
RunStep()base.js (line 114)

Comment 5

[Hannes Verschore [:h4writer]]

Scratch that, the first argument to bnpSquareTo could be nbi(), i.e. no "r.t" or "r.s" set. So we have multiple objectshapes running through that argument, BUT we decide there can only be one, because MJIT caches report that.

So the big question is why do MJIT caches report only one particular shape running there??? Note that at the end of the bnpSquareTo function (so after adding/setting the values), the shape will always be the same. Does MJIT know this and optimize or something???

Comment 6

[Hannes Verschore [:h4writer]]

Okay, found it, now with 100% certainty. The problem is actually what I reported when opening the bug. A type is only seen 1 out of 10000 times. Therefore MJIT caches report them as single typed. Just as we enter IM we see the other type and bail :(.

bnpSquareTo: 504 times called with "type 1"
bnpSquareTo loops: 18 times
=> usecount 9072, compile IM
bnpSquareTo: 2 times called with "type 2"
=> IM bail
REPEAT

I think eventually we want to be able to disable looking at MJIT caches on a per set/get basis. For now I think we just have to disable them on a function base. As already reported we have to be carefull here, as it is easy to regress JSNES doing this

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Possible fix V2

In this patch we disable retrieving information from mjit caches for getprop/setprop seperately. Also shapeguard is adjusted to guard for the whole shapetree till the needed property in ool path.

h4writer@h4writer-ThinkPad-W520:~/Build/mozilla-inbound/js/src/v8$ mi64; js run.js
Richards: 3032
DeltaBlue: 2992
Disable setprop crypto.js:431
Disable getprop crypto.js:243
Crypto: 4337
RayTrace: 2612
EarleyBoyer: 3894
RegExp: 528
Splay: 3121
NavierStokes: 5702
----
Score (version 7): 2799
h4writer@h4writer-ThinkPad-W520:~/Build/mozilla-inbound/js/src/v8$ mi64; js run.js
Richards: 3036
DeltaBlue: 3379
Disable setprop crypto.js:431
Disable getprop crypto.js:243
Disable getprop crypto.js:415
Crypto: 4511
RayTrace: 2415
EarleyBoyer: 3882
RegExp: 530
Splay: 3256
NavierStokes: 5702
----
Score (version 7): 2843
h4writer@h4writer-ThinkPad-W520:~/Build/mozilla-inbound/js/src/v8$ mi64; js run.js
Richards: 3040
DeltaBlue: 3405
Disable setprop crypto.js:431
Disable getprop crypto.js:415
Disable getprop crypto.js:243
Crypto: 4556
RayTrace: 2391
EarleyBoyer: 3893
RegExp: 526
Splay: 3183
NavierStokes: 5691
----
Score (version 7): 2836
h4writer@h4writer-ThinkPad-W520:~/Build/mozilla-inbound/js/src/v8$ mi64-2; js run.js
Richards: 2863
DeltaBlue: 2870
Crypto: 4583
RayTrace: 2580
EarleyBoyer: 3805
RegExp: 524
Splay: 3105
NavierStokes: 5719
----
Score (version 7): 2767
h4writer@h4writer-ThinkPad-W520:~/Build/mozilla-inbound/js/src/v8$ mi64-2; js run.js
Richards: 2793
DeltaBlue: 3157
Crypto: 3898
RayTrace: 2503
EarleyBoyer: 3760
RegExp: 509
Splay: 3053
NavierStokes: 5657
----
Score (version 7): 2702
h4writer@h4writer-ThinkPad-W520:~/Build/mozilla-inbound/js/src/v8$ mi64-2; js run.js
Richards: 2785
DeltaBlue: 3263
Crypto: 3855
RayTrace: 2630
EarleyBoyer: 3876
RegExp: 530
Splay: 3121
NavierStokes: 5696
----
Score (version 7): 2760
h4writer@h4writer-ThinkPad-W520:~/Build/mozilla-inbound/js/src/v8$ mi64; js run.js
Richards: 3043
DeltaBlue: 3408
Disable setprop crypto.js:431
Disable getprop crypto.js:415
Disable getprop crypto.js:243
Crypto: 4568
RayTrace: 2459
EarleyBoyer: 3883
RegExp: 525
Splay: 3102
NavierStokes: 5696
----
Score (version 7): 2837

That's a 2% improvement on v8. Crypto is now stable at 4568, instead of between 3800-4500. Raytrace regresses and Deltablue and Richards gain a lot. Not sure why.

JSNES regresses 10% though :(

Comment 8

[Hannes Verschore [:h4writer]]

Since JM will get removed pretty soon (around 13 May, after ff23 merge to aurora)