1353170 - Implement recover instructions for NewArrayIterator

Status: RESOLVED FIXED

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

Description

[Tom S [:evilpie]]

Attachment: Implement recover instructions for NewArrayIterator

I implemented recover instructions, but we don't actually seem to remove the allocation on the micro benchmark "destructuring.es6" from six-speed. I guess something else is keeping the object alive? We are reading/writing fixed slots on that object.

Nicolas, maybe you can take a look and point me in the right direction?

Comment 1

[Tom S [:evilpie]]

Attachment: Implement recover instructions for NewArrayIterator

Comment 2

[Tom S [:evilpie]]

Attachment: micro benchmark

Comment 3

[Jan de Mooij [:jandem]]

You also have to handle this instruction in ScalarReplacement in ScalarReplacement.cpp I think? Search for `CreateThisWithTemplate` for instance.

Comment 4

[Tom S [:evilpie]]

Attachment: Implement scalar replacement for NewArrayIterator

This works on the destructuring benchmark and improves our time from 30ms to 10ms \o/. Thanks for the tip Jan!

I will look into writing some tests for this with assertRecoveredOnBailout.

Comment 5

[Tom S [:evilpie]]

Attachment: Implement scalar replacement for NewArrayIterator

Try looks good enough: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca9d874120bf968a8ec27074e5484e71cec72c48

This is hard to test: array[Symbol.iterator]() might not be inlined, so we can't use scalar replacement. destructuring and for-of don't expose the iterator.

I am not sure yet if this is going to work with for-of, often we fail to do because of OSR (I think), the object is being leaked by a PHI instruction.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8854590 [details] [diff] [review]
Implement scalar replacement for NewArrayIterator

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

Great results!

::: js/src/jit/Recover.h
@@ +596,5 @@
>  
> +class RNewArrayIterator final : public RInstruction
> +{
> +  private:
> +    uint32_t count_;

We can remove this field IIUC.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df2d145e4c40
Implement scalar replacement for NewArrayIterator. r=jandem

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2bb2f9f7dc3
disable the other assertRecoveredOnBailout as well.

Comment 9

[Tom S [:evilpie]]

I was hoping for a bigger wins on destructuring/for-of, but maybe we only inline everything that is needed later. Or maybe the PHI problem strikes again. I am going to file two followup ups: assertRecoveredOnBailout is hard to use and we should figure out the phi thing.

Comment 10

[Jan de Mooij [:jandem]]

The getSelfHostedValue version fails with --no-threads because we have an any-object typeset for the result of getSelfHostedValue. You can fix that by moving this line out of the function:

  var NewArrayIterator = getSelfHostedValue("NewArrayIterator");

With off-thread compilation the result depends on whether the main thread is faster than the compilation thread.

The iterator test fails because of some code in ArrayIteratorNext:

(1) The UnsafeSetReservedSlot if index >= len. This one we can probably fix by inlining better or by improving branch pruning heuristics?

(2) The CallArrayIteratorMethodIfWrapped call. We should know |!IsObject(this) || !IsArrayIterator(this)| is always false so we should be able to optimize/prune this somehow.

Maybe we can check if removing these branches helps sixspeed.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df2d145e4c40
https://hg.mozilla.org/mozilla-central/rev/c2bb2f9f7dc3