784127 - IonMonkey: Bailing from GuardShape from JM caches must update JM caches

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

The recurring bailout in JSNES happens as follows:

> Compile :289 while reading a single shape from a JM cache, placing MGuardShape;
> During IM execution, a different shape flows in, causing bailout from MGuardShape;
> Run in interpreter, JM doesn't change cache state;
> Recompile with old JM cache information, guard against the wrong Shape.

In general, IM bailouts require some state to be mutated to guarantee that next recompilation doesn't include the erroneous assumption. Normally this occurs through TI freezes, which is updated by the interpreter and therefore requires no further action.

In this case, we should clear the JM cache to resolve the incorrect single-shape assumption.

Comment 1

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

Do we also need to invalidate if this happens too many times?

Comment 2

[Sean Stangl [:sstangl]]

New theory for new evidence.

IonMonkey currently reads from JaegerMonkey caches in the same way that v8's Crankshaft reads from its base JIT's caches: we look at the target script, find the PIC with the appropriate PC, and read its contents. However, JM and v8's base JIT have an important difference: v8's base JIT does not support function inlining.

So when Crankshaft compilation reads from a base JIT cache, it knows that it's looking at a list of plausible shapes for when that function is called from the context of any function. In our situation, however, IonMonkey can't know *which* cache to use, since due to inlining JaegerMonkey may have many for the same PC:

> One for when the function is directly compiled,
> One for when the function is inlined in the context of caller A,
> One for when the function is inlined in the context of caller B,
> And in the darkness, bind them.

This works perfectly fine in the (benchmark) ultra-monomorphic case where shapes are equal in all contexts. Unfortunately for the polymorphic case in JSNES, even if we invalidate the cache associated with the failing GuardShape, the cache may still be later filled in with an invalid Shape for the context we expect.

Therefore, in case of JM-derived ShapeGuard error, we should disable optimizations that read from JM caches for the target script.

Comment 3

[Sean Stangl [:sstangl]]

(In reply to David Anderson [:dvander] from comment #1)
> Do we also need to invalidate if this happens too many times?

LGuardShape always invalidates, because there's no way for TI to freeze on shapes. Hopefully the suggestion in Comment 2 prevents this from happening too many times; Bug 784134 would be an additional, reasonable safeguard.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

This is the fix proposed in comment 2: set a JSScript bit if invalidation was triggered by a shape guard and next time don't inline property accesses if this bit is set for the current script or an outer script.

JM inlining is disabled now but this was still a problem. For instance, if there is a conditional property access in a loop, we may inline the shape guard and LICM can then hoist it to the loop header. But if the guard fails, the JM cache won't necessarily be updated since the property access may be inside an if-statement that's not always executed.

With this patch --no-ion is still 60 ms faster (453 ms vs 390 ms) on the shell benchmark on 32-bit, but I get the same numbers if I disable monomorphic inlining altogether so that seems unrelated to this bug.

Comment 5

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/projects/ionmonkey/rev/d11dafc10fc0

Comment 6

[Sean Stangl [:sstangl]]

I didn't test this patch's effect on benchmark performance -- although it eliminates about 4 seconds of compilation time, it actually regresses our performance on the later frames from 550ms -> 780ms, and the shell benchmark in its entirety from 855 -> 900ms.

Comment 7

[Sean Stangl [:sstangl]]

Attempting to remedy the problem by using failedCachedShapeGuard_ to only prevent hoisting further regresses benchmark perf to 1010ms, later frames 610ms.

Comment 8

[Jan de Mooij [:jandem]]

Weird, I get these numbers on 32-bit for shell-bench.js (the attachment in bug 783143):

Revision 07d466a718a6 (tip - 1):

warmup: 431
warmup: 1415
warmup: 1398
result: 438
result: 1852
result: 1847
result: 471
result: 437
result: 439
result: 430
result: 428
result: 435
result: 383
Average of 10 runs: 716.00ms.

Revision d11dafc10fc0 (tip):

warmup: 438
warmup: 438
warmup: 460
result: 525
result: 568
result: 469
result: 432
result: 400
result: 398
result: 399
result: 393
result: 395
result: 512
Average of 10 runs: 449.10ms.

Note that --no-ion is faster:

warmup: 392
warmup: 380
warmup: 422
result: 470
result: 532
result: 486
result: 343
result: 335
result: 336
result: 334
result: 335
result: 334
result: 423
Average of 10 runs: 392.80ms.

I will try a 64-bit build now, otherwise I don't know why we get different results...

Comment 9

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/projects/ionmonkey/rev/e164663c4b6f

Backed out since it regresses actual JSNES performance. Apparently this optimization is too critical to performance to disable.

I didn't see Jan's comment before backing out. I was testing on 64-bit; will try local numbers for 32-bit.

Comment 10

[Sean Stangl [:sstangl]]

x86_64 without patch:
warmup: 721
warmup: 2087
warmup: 2060
result: 625
result: 2412
result: 1666
result: 564
result: 537
result: 549
result: 535
result: 527
result: 597
result: 568
Average of 10 runs: 858.00ms.

x86_64 with patch:
armup: 700
warmup: 750
warmup: 905
result: 1117
result: 1380
result: 847
result: 798
result: 787
result: 789
result: 790
result: 789
result: 853
result: 806
Average of 10 runs: 895.60ms.

x86_64 with --no-ion:
warmup: 709
warmup: 730
warmup: 854
result: 1007
result: 1203
result: 743
result: 712
result: 705
result: 716
result: 707
result: 706
result: 668
result: 666
Average of 10 runs: 783.30ms.

x86 without patch:
warmup: 617
warmup: 2006
warmup: 2001
result: 568
result: 2467
result: 2488
result: 565
result: 527
result: 525
result: 512
result: 511
result: 511
result: 522
Average of 10 runs: 919.60ms.

x86 with patch:
warmup: 605
warmup: 579
warmup: 635
result: 744
result: 849
result: 645
result: 577
result: 537
result: 531
result: 542
result: 533
result: 539
result: 681
Average of 10 runs: 617.80ms.

x86 --no-ion:
warmup: 571
warmup: 523
warmup: 589
result: 691
result: 786
result: 697
result: 475
result: 462
result: 465
result: 465
result: 472
result: 464
result: 569
Average of 10 runs: 554.60ms.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Different approach. This patch stores a list of compiled/inlined JSScript's inside the IonScript and when a shape guard fails we purge JM caches in these scripts. I can't think of any alternative other than the previous patch.

Sean, can you see if this patch actually works for you on 64-bit? My results show that this patch (p2) is a win over the previous patch (p1) and tip, but I wasn't able to reproduce the slowdown with the previous patch, so I'd like to double check it this time before I continue.

32-bit:

        tip   p1   p2   --no-ion
warmup:  453  473  459  401
warmup: 1391  463  367  380
warmup: 1379  486  366  422
result:  435  548  381  473
result: 1994  587  366  530
result: 1999  469  414  479
result:  460  424  392  340
result:  403  402  378  328
result:  400  401  381  331
result:  389  403  383  327
result:  390  399  385  340
result:  391  399  378  328
result:  414  515  434  429

avg:     727  455  389  390

64-bit:

        tip   p1   p2   --no-ion
warmup:  540  543  543  538
warmup: 1452  579  463  547
warmup: 1475  677  457  633
result:  543  799  472  827
result: 2062  955  449  945
result: 1417  652  511  559
result:  461  641  478  534
result:  443  610  470  528
result:  445  614  474  530
result:  430  614  470  528
result:  430  613  473  529
result:  466  633  511  567
result:  455  604  478  538

avg:     727  673  478  608

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 656839 [details] [diff] [review]
Patch v2

Setting feedback flag per comment 11.

Comment 13

[Sean Stangl [:sstangl]]

Local results:

x86

         tip   p2  tip --no-ion
warmup:  635  641  639
warmup: 1981  488  510
warmup: 1999  486  585
result:  574  498  657
result: 2717  479  759
result: 2709  507  682
result:  593  476  489
result:  520  460  494
result:  517  465  475
result:  495  461  473
result:  493  463  483
result:  491  456  475
result:  536  560  576

avg:     964  482  556


x86_64

         tip   p2  tip --no-ion
warmup:  717  723  812
warmup: 1982  579  760
warmup: 2028  585  897
result:  678  592  1038
result: 2729  587  1255
result: 1881  640  721
result:  567  593  682
result:  544  585  692
result:  552  588  673
result:  533  589  672
result:  530  584  703
result:  590  645  699
result:  578  597  657

avg:     918  600  779

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Patch v3

Cool, thanks for testing!

This patch does not mark the scripts stored in the IonScript and MIRGraph vector. We rely already on JSScript not moving (see the comment in CodeGeneratorX86::visitRecompileCheck for details) and every MBasicBlock has a JSScript pointer (in CompileInfo).

Comment 15

[Sean Stangl [:sstangl]]

Comment on attachment 660096 [details] [diff] [review]
Patch v3

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

::: js/src/ion/IonCode.h
@@ +339,2 @@
>      size_t size() const {
> +        return scriptList_ + scriptEntries_ * sizeof(JSScript *);

Alternatively, we could put the script list before the safepoints list.

Comment 16

[Sean Stangl [:sstangl]]

Let's get it landed.

Comment 17

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9857e5cfa42b

Try run: https://tbpl.mozilla.org/?tree=Try&rev=e80ee5a7a20f

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9857e5cfa42b