1365782 - [Ion] MConcat doesn't handle throw correctly

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Ted Campbell [:tcampbell]]

Attachment: MConcat_Allocation.js

We currently throw a catchable exception when a concatenation would allocate a very large string. In this example, under Ion, the throw escapes the try block whereas on baseline it is caught as expected.

Comment 1

[Ted Campbell [:tcampbell]]

In this example, the MConcat is marked as movable, and LICM moves it outside the try block so exception is reported as firing from line 4.

The naive fix would be to no longer mark as moveable, but we should measure the performance impact of this.

Comment 2

[Ted Campbell [:tcampbell]]

The second bug this uncovers is that we do not use a resumeAfter in |IonBuilder::binaryArithTryConcat| which causes |x| to have a stale value in the catch block.

Comment 3

[Tom S [:evilpie]]

Having a movable string concat seems like quite an useful optimization to me. Especially because we don't nursery allocate strings. In general we should be allowed to make this optimization, because internal errors are not specified, but the wrong line number is obviously confusing.

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: MConcat_Speculative.js

Ah, good point. This brings up another test case where we have dynamic guards around the concat. The guards would prevent the very large concat from executing. If it gets hoisted, we will throw.

Comment 5

[Ted Campbell [:tcampbell]]

Proposed solution for this is to use ConcatStrings<NoGC> and bailout if it fails. This also solves the problem of a MConcat that might OOM being hoisted from inside an if block that wouldn't execute at runtime.

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Bug 1365782 - Bailout from MConcat instead of throwing

Review commit: https://reviewboard.mozilla.org/r/140846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140846/

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8869287 [details]
Bug 1365782 - Bailout from MConcat instead of throwing

https://reviewboard.mozilla.org/r/140846/#review145120

Makes sense.

::: js/src/jit-test/tests/ion/bug1365782-1.js:11
(Diff revision 1)
> +            var x = 1;
> +            Array(1);
> +            x = 2;
> +            x = t + t; // May throw if too large
> +        } catch (e) {
> +            assertEq(x, 2);

Maybe add a global variable var threw = 0; then here we can do threw++ and at the end of the file assertEq(threw, ..) ?

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js:1386
(Diff revision 1)
> -    rconcat_string(i);
> -    rconcat_number(i);
> + // rconcat_string(i);  // Disabled by Bug 1365782
> + // rconcat_number(i);  // Disabled by Bug 1365782

This is a bit unfortunate but it's probably ok. We should check if it affects AWFY though.

::: js/src/jit/CodeGenerator.cpp:7451
(Diff revision 1)
>      masm.call(stringConcatStub);
> -    masm.branchTestPtr(Assembler::Zero, output, output, ool->entry());
> +    masm.branchTestPtr(Assembler::NonZero, output, output, &done);
>  
> +    // If the concat would otherwise throw, we instead return nullptr and use
> +    // this to signal a bailout. This allows MConcat to be movable.
> +    masm.jmp(ool->entry());

s/jmp/jump, not sure if all platforms define jmp

::: js/src/jit/VMFunctions.cpp:1794
(Diff revision 1)
>      JSType type = js::TypeOfObject(obj);
>      return TypeName(type, *rt->commonNames);
>  }
>  
> +bool
> +ConcatStringsPure(JSContext* cx, JSString* lhs, JSString* rhs, MutableHandleString res)

JSString** does not work I suppose?

::: js/src/jit/VMFunctions.cpp:1801
(Diff revision 1)
> +    // ConcatStrings without GC or throwing. If this fails, we set result to
> +    // nullptr and let caller do a bailout. Return true to indicate no exception
> +    // thrown.
> +    JSString* result = ConcatStrings<NoGC>(cx, lhs, rhs);
> +    res.set(result);
> +    return true;

Can MOZ_ASSERT(!cx->isExceptionPending()); here.

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 8869287 [details]
Bug 1365782 - Bailout from MConcat instead of throwing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140846/diff/1-2/

Comment 9

[Ted Campbell [:tcampbell]]

Marking leave-open until we see AWFY impact.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/882d55c60444
Bailout from MConcat instead of throwing r=jandem

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/882d55c60444

Comment 12

[Ted Campbell [:tcampbell]]

six-speed-templatestring-es6 is 10x slower by this. Need to reinvestigate. REOPENING.

Comment 13

[Ted Campbell [:tcampbell]]

Looks like this is another no-op benchmark that we were doing well at by DCE-ing the MConcat. Once we mark as setGuard to prevent different exception behavior from baseline, we slow down heavily.

Comment 14

[Ted Campbell [:tcampbell]]

After some discussion with :nbp and :jandem, it seems best to remove the setGuard. The InternalError exception is an artifact of our JString implementation and we shouldn't expect it be thrown if we can otherwise eliminate the operation.

This will restore the six-speed perf, while still maintaining exceptions that do occur to appear in the correct location.

Comment 15

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1365782-Allow-MConcat-to-DCE.patch

Fix AWFY regressions. Allow MConcat to be eliminated if result unused.

The MConcat may throw on OOM or JString length overflow. These are both implementation details that are subject to change and shouldn't be expected to throw if we can avoid them.

If they do throw, we ensure it happens where the user would expect it, even if the operation was moved by optimizations.

Comment 16

[Ted Campbell [:tcampbell]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4786460b0c7b4516508c43318002cebea3367b03

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out at Ted's request so it doesn't add extra risk to 55 going into Beta.

https://hg.mozilla.org/integration/mozilla-inbound/rev/09084e1e008f1390659eced83bbea2b0a30644a7

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This backout appears to have caused cgc tests to permafail: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&noautoclassify&fromchange=82ffae610a03a314544f54d6d79edafb3f0daa7f&filter-searchStr=709af0b1dfa7d4b188ae77714302b60db9a4ccf2&tochange=b693eb4b9ef6967c8a73e67e35df80705653ee2f&selectedJob=105554944

Any chance of a quick fix or should I backout the backout?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1e9ea66c7f
Backout the backout of changeset 882d55c60444 because of cgc bustage because current bustage is worse than potential future risk a=bustage

Comment 20

[Carsten Book [:Tomcat]]

also backedout from m-c
https://hg.mozilla.org/mozilla-central/rev/09084e1e008f

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c1e9ea66c7f

Comment 22

[Ted Campbell [:tcampbell]]

Okay, I looked into the cgc failure. It looks like the failing test case is just too slow for cgc mode and should be added to cgc-jittest-timeouts.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ea272d7d2ae49db2eb42052f714786f7f9c056c

Comment 23

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1365782-Revert-Bailout-from-MConcat-instead-of-t.patch

Revert original patch and add test timeout exceptions.

This returns us to original code state to avoid risk to upcoming beta55.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c26e124516f4
Backed out changeset 882d55c60444 to avoid adding risk to Beta55.
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ab93e49fa8
Disable jit-test/parser/bug-1355046.js on cgc for timeouts. r=test-only

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/06ab93e49fa8

Comment 26

[Ted Campbell [:tcampbell]]

Updating tracking. These patches have been reverted and original issue remains.

The throw bug involves large strings allocations throwing an OOM where interpreter would otherwise not require an allocation. Marking existing versions as wontfix since there is no external indicators this affects people.

Revisit for FF56 and see if there is a more stable fix, or if these implementation detail exceptions are not worth improving.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Target Milestone is used to track when a fix lands on m-c in general, so let's leave it un-set for now.

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:tcampbell, maybe it's time to close this bug?

Comment 29

[Ted Campbell [:tcampbell]]

This was a mess that in the end was not worth landing. Nothing is "wrong" here, just might throw a catchable OOM sooner than expected. There was never any report of this in the wild.