Note: There are a few cases of duplicates in user autocompletion which are being worked on.

IonMonkey: Add fast paths for iterators

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Bug 701965 added slow paths, but we should also add fast paths.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 604396 [details] [diff] [review]
Patch
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+
(Assignee)

Comment 4

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.