Last Comment Bug 725241 - IonMonkey: Add fast paths for iterators
: IonMonkey: Add fast paths for iterators
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
Depends on:
Blocks: IonSpeed
  Show dependency treegraph
 
Reported: 2012-02-08 02:13 PST by Jan de Mooij [:jandem]
Modified: 2012-03-12 02:38 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (20.64 KB, patch)
2012-03-08 11:31 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch (37.47 KB, patch)
2012-03-09 06:37 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-02-08 02:13:19 PST
Bug 701965 added slow paths, but we should also add fast paths.
Comment 1 Jan de Mooij [:jandem] 2012-03-08 11:31:19 PST
Created attachment 604137 [details] [diff] [review]
WIP

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.
Comment 2 Jan de Mooij [:jandem] 2012-03-09 06:37:54 PST
Created attachment 604396 [details] [diff] [review]
Patch
Comment 3 David Anderson [:dvander] 2012-03-09 17:40:02 PST
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().
Comment 4 Jan de Mooij [:jandem] 2012-03-12 02:38:48 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.