Closed Bug 1353170 Opened 6 years ago Closed 6 years ago

Implement recover instructions for NewArrayIterator


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




Tracking Status
firefox55 --- fixed


(Reporter: evilpie, Assigned: evilpie)


(Blocks 2 open bugs)



(2 files, 3 obsolete files)

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?
Attachment #8854159 - Flags: feedback?(nicolas.b.pierron)
Attachment #8854159 - Attachment is patch: true
Attachment #8854159 - Attachment is obsolete: true
Attachment #8854159 - Flags: feedback?(nicolas.b.pierron)
Attachment #8854160 - Flags: feedback?(nicolas.b.pierron)
Attached file micro benchmark
Assignee: nobody → evilpies
You also have to handle this instruction in ScalarReplacement in ScalarReplacement.cpp I think? Search for `CreateThisWithTemplate` for instance.
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.
Attachment #8854160 - Attachment is obsolete: true
Attachment #8854160 - Flags: feedback?(nicolas.b.pierron)
Try looks good enough:

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.
Attachment #8854373 - Attachment is obsolete: true
Attachment #8854590 - Flags: review?(jdemooij)
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.
Attachment #8854590 - Flags: review?(jdemooij) → review+
Pushed by
Implement scalar replacement for NewArrayIterator. r=jandem
Pushed by
disable the other assertRecoveredOnBailout as well.
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.
Blocks: 1353863
Blocks: 1353875
Flags: needinfo?(jdemooij)
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.
Flags: needinfo?(jdemooij)
Priority: -- → P3
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.