Closed
Bug 784127
Opened 12 years ago
Closed 12 years ago
IonMonkey: Bailing from GuardShape from JM caches must update JM caches
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: sstangl, Assigned: jandem)
References
Details
(Whiteboard: [ion:p2:fx18])
Attachments
(1 file, 2 obsolete files)
28.36 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
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.
Do we also need to invalidate if this happens too many times?
Whiteboard: [ion:p2:fx18]
Reporter | ||
Comment 2•12 years ago
|
||
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.
Reporter | ||
Comment 3•12 years ago
|
||
(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.
Assignee | ||
Comment 4•12 years ago
|
||
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.
Reporter | ||
Updated•12 years ago
|
Attachment #655969 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/d11dafc10fc0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 6•12 years ago
|
||
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.
Reporter | ||
Comment 7•12 years ago
|
||
Attempting to remedy the problem by using failedCachedShapeGuard_ to only prevent hoisting further regresses benchmark perf to 1010ms, later frames 610ms.
Assignee | ||
Comment 8•12 years ago
|
||
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...
Reporter | ||
Comment 9•12 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 10•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
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
Attachment #655969 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
Comment on attachment 656839 [details] [diff] [review] Patch v2 Setting feedback flag per comment 11.
Attachment #656839 -
Flags: feedback?(sstangl)
Reporter | ||
Comment 13•12 years ago
|
||
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
Reporter | ||
Updated•12 years ago
|
Attachment #656839 -
Flags: feedback?(sstangl) → feedback+
Assignee | ||
Comment 14•12 years ago
|
||
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).
Attachment #656839 -
Attachment is obsolete: true
Attachment #660096 -
Flags: review?(sstangl)
Reporter | ||
Comment 15•12 years ago
|
||
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.
Attachment #660096 -
Flags: review?(sstangl) → review+
Reporter | ||
Comment 16•12 years ago
|
||
Let's get it landed.
Assignee | ||
Comment 17•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9857e5cfa42b Try run: https://tbpl.mozilla.org/?tree=Try&rev=e80ee5a7a20f
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9857e5cfa42b
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•