1806743 - Add MIRType::Iterator

Status: NEW

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

Description

[Iain Ireland [:iain]]

In bug 1799025, we improved our optimization of for-in iterators in Ion. Iterators in Ion are represented as a PropertyIteratorObject wrapping a NativeIterator, which means that we have to load the NativeIterator out of the wrapper every time we touch it. As a follow up, we could consider adding MIRType::Iterator and using an unwrapped NativeIterator instead.

Comment 1

[Iain Ireland [:iain]]

Attachment: WIP: Bug 1806743: Add MIRType::NativeIterator

Comment 2

[Iain Ireland [:iain]]

Attachment: WIP: Bug 1806743: Add MUnwrapIterator

I initially intended to change ValueToIterator and ObjectToIterator to return MIRType::NativeIterator, but that change has to be plumbed through code to rewrap the native iterator when reading the snapshot. After writing most of that patch, I realized that it was much simpler to only change the consumers, let the TypePolicy insert the necessary conversions, and keep the snapshot unchanged. In practice, we end up with a single UnwrapIterator hoisted up just below the ValueToIterator/ObjectToIterator.

Depends on D165478

Comment 3

[Iain Ireland [:iain]]

Attachment: WIP: Bug 1806743: Use MIRType::NativeIterator for IteratorHasIndices

Depends on D165479

Comment 4

[Iain Ireland [:iain]]

Attachment: WIP: Bug 1806743: Use MIRType::NativeIterator for LoadSlotByIteratorIndex

Depends on D165480

Comment 5

[Iain Ireland [:iain]]

Attachment: WIP: Bug 1806743: Refactor LoadNativeIterator

This simplifies the next two patches.

Depends on D165481

Comment 6

[Iain Ireland [:iain]]

Attachment: WIP: Bug 1806743: Use MIRType::NativeIterator for IteratorMore

Depends on D165482

Comment 7

[Iain Ireland [:iain]]

Attachment: WIP: Bug 1806743: Use MIRType::NativeIterator for IteratorEnd

This has the side-effect of letting us simplify the register allocation for the baseline implementation.

Depends on D165483

Comment 8

[Iain Ireland [:iain]]

I implemented this, but I didn't see any measurable performance improvements. Not sure if it's worth landing the patches, but I put them up as a WIP.

Comment 9

[Jan de Mooij [:jandem]]

We have to be a bit careful with GC: if an iterator can outlive its owning JS object, we'd have a UAF. For object Slots/Elements we have this insert-keepalive-instructions pass.

Comment 10

[Iain Ireland [:iain]]

My reasoning for not including keepalive instructions in these patches was that the owning JS object is always being kept alive in the snapshot because it's used on bailout. In the slots/elements case, you can write code where the object whose slots/elements are being accessed is a temporary value that is unreachable at the point of the snapshot. In the iterator case, the iterator object is on the operand stack for the duration of the loop, so (I think/hope) it should always be included in the snapshot.

But it's possible that I'm missing something.