1804759 - Asserting in EmberJS-Debug-TodoMVC does a lot of work to build the error message

Status: NEW

(Core :: JavaScript Engine, defect, P2)

Description

[Alex Thayer [:alexical] (she/her)]

In EmberJS-Debug-TodoMVC, we spend almost 15% of our time in this line:

_emberMetalDebug.assert('When using @each to observe the array ' + content + ', the array must return an object', typeof item === 'object');

The expensive part of this is that content is often a large array of objects, so we toString it, which takes a lot of time, and do two string concats, which take less, but still measurable time.

I talked with Iain about this, and he suggested that because the assert never fails, we will generate an unconditional bailout for the site that actually uses the string, and so we should be able to defer some of the work until bailout. A few notes on this:

• CallStringObjectConcatResult is not wired up to be transpiled, and so we end up generating an Ion IC for it. Fixing this should be trivial, and may help us a little bit in certain places.
• We could go further though by adding something like toStringPure which would guarantee that it is not effectful, and split CallStringObjectConcatResult into ToStringPure and StringConcatResult, allowing us to actually move the concat to bailout time. However, the ToStringPure would still be difficult to move to bailout time because we would have to guarantee that its input value isn't changed from underneath us.
• Because we can't rely on pushing the toString cost to bailout time, we should reinvestigate our Sink pass which could move this code.

Comment 1

[Nicolas B. Pierron [:nbp]]

Branch Pruning might also be sufficient in this case, if the else-case of the assertion is not generated.

Comment 2

[Jan de Mooij [:jandem]]

Some of these operations are pretty complicated due to potential side-effects. It's probably worth micro-benchmarking this pattern a bit against V8 and/or JSC to see where they are doing better than us.

Comment 3

[Iain Ireland [:iain]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)

Branch Pruning might also be sufficient in this case, if the else-case of the assertion is not generated.

Branch pruning is necessary, but not sufficient, because the assertion message is built outside the else-case. We already recover the second string concatenation on bailout, but the first one isn't visible to Ion. (The CallStringObjectConcatResult isn't transpiled, so we use an Ion IC.) If we can represent the first concatenation as two MIR nodes (one for the toString, one for the concat), then we can at least move all the string concatenation to the bailout path.

Moving the toString out of the fast path is harder, which is why I speculated that it might be worth looking at Sink again. The hard problem that we would need to solve is whether there's any way to tell that toString doesn't have any side effects without actually running it.

Comment 4

[Iain Ireland [:iain]]

For reference, bug 1093674 is where we implemented and then disabled large chunks of Sink, and bug 1109195 is theoretically tracking the work to reenable it (although it hasn't been meaningfully touched in 8 years).