Closed Bug 725241 Opened 12 years ago Closed 12 years ago

IonMonkey: Add fast paths for iterators

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 701965 added slow paths, but we should also add fast paths.
Attached patch WIP (obsolete) — Splinter Review
First stab. Passes tests on x86 and improves string-fasta from 20 to 13 ms. Still need to clean this up, rename a few things and add more comments.
Attached patch PatchSplinter Review
Attachment #604137 - Attachment is obsolete: true
Attachment #604396 - Flags: review?(dvander)
Comment on attachment 604396 [details] [diff] [review]
Patch

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

::: js/src/ion/CodeGenerator.cpp
@@ +1523,5 @@
> +    // Iterators other than for-in should use LCallIteratorStart.
> +    JS_ASSERT(flags == JSITER_ENUMERATE);
> +
> +    // Fetch the most recent iterator and ensure it's not NULL.
> +    void *lastIterator = &gen->info().script()->compartment()->nativeIterCache.last;

This can just be cx->compartment

@@ +1558,5 @@
> +    masm.storePtr(obj, Address(niTemp, offsetof(NativeIterator, obj)));
> +    masm.or32(Imm32(JSITER_ACTIVE), Address(niTemp, offsetof(NativeIterator, flags)));
> +
> +    // Chain onto the active iterator stack.
> +    masm.loadJSContext(GetIonContext()->cx->runtime, temp1);

Not related to this patch, but I just noticed loadJSContext doesn't need that JSRuntime parameter (we can have it call GetIonContext())

@@ +1601,5 @@
> +    LoadNativeIterator(masm, obj, temp, ool->entry());
> +
> +    // Get cursor, next string.
> +    masm.loadPtr(Address(temp, offsetof(NativeIterator, props_cursor)), output.scratchReg());
> +    masm.loadValue(JSVAL_TYPE_STRING, Address(output.scratchReg(), 0), output);

This new loadValue overload should be called "loadAndTag" or something, since it's doing more than just loading. Or, if it works, we should just emit two masm calls, one to load and another to box.

@@ +1665,5 @@
> +    masm.storePtr(temp1, Address(temp2, offsetof(JSContext, enumerators)));
> +
> +    // Update IonActivation::savedEnumerators, see CloseIteratorFromIon.
> +    Label done;
> +    masm.loadIonActivation(cx->runtime, temp2); // XXX: Is this load necessary?

Yeah, I think it is.

@@ +1670,5 @@
> +
> +    Address savedEnumerators(temp2, IonActivation::offsetOfSavedEnumerators());
> +    masm.branchPtr(Assembler::NotEqual, savedEnumerators, obj, &done);
> +    {
> +        masm.storePtr(temp1, savedEnumerators);

Not related to this patch, but it turns out our savedEnumerators thing is not really correct. I'm going to try using try notes to get rid of savedEnumerators, so hopefully this can go away later.

::: js/src/ion/Lowering.cpp
@@ +1346,5 @@
> +        return defineVMReturn(lir, ins) && assignSafepoint(lir, ins);
> +    }
> +
> +    LIteratorStart *lir = new LIteratorStart(useRegister(ins->object()), temp(LDefinition::GENERAL),
> +                                             temp(LDefinition::GENERAL), temp(LDefinition::GENERAL));

If this was annoying to type (it looks like it was), you could make tempGeneral() or just temp().
Attachment #604396 - Flags: review?(dvander) → review+
Pushed with all comments addressed:

https://hg.mozilla.org/projects/ionmonkey/rev/bf6acad353e0

(In reply to David Anderson [:dvander] from comment #3)
>
> This new loadValue overload should be called "loadAndTag" or something,
> since it's doing more than just loading. Or, if it works, we should just
> emit two masm calls, one to load and another to box.

I changed it to call loadPtr and tagValue.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.