Bug 122575 – Further improve ArrayIterator performance

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Further improve ArrayIterator performance

[Oliver Hunt]

Created attachment 213817 [details]
Patch

[Oliver Hunt]

Created attachment 213821 [details]
Patch

[Nadav Rotem]

You added whitespace in  Source/JavaScriptCore/runtime/JSGlobalObject.cpp

Can you please document arrayIteratorNextThunkGenerator(VM* vm)  ?

[Oliver Hunt]

Created attachment 213926 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 213926 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213926&action=review

> Source/JavaScriptCore/jit/SpecializedThunkJIT.h:47
> +            if (expectedArgCount >= 0) {
> +                // Check that we have the expected number of arguments
> +                m_failures.append(branch32(NotEqual, payloadFor(JSStack::ArgumentCount), TrustedImm32(expectedArgCount + 1)));
> +            }

Is there a better way we could indicate that we shouldn't do an arg count check than just passing -1? Maybe an enum with a default value set to emit the check?

> Source/JavaScriptCore/jit/SpecializedThunkJIT.h:68
> +        void loadSpecificClassArgument(const ClassInfo* classInfo, int argument, RegisterID dst, RegisterID scratch)

Name is sort of confusing. loadArgumentWithClassCheck?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1018
> +    // Return the termination signal to indicate that we've finished

Move comment down one line, or change to indicate we're doing a check and then possibly returning.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1027
> +    Jump notKey = jit.branch32(SpecializedThunkJIT::NotEqual, Address(SpecializedThunkJIT::regT4, JSArrayIterator::offsetOfIterationKind()), TrustedImm32(ArrayIterateKey));

Are we sure the cmp/branch here is the right size? Should it be a branch8 instead? I guess alignment will save us in this case, but I thought I'd just make sure.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1048
> +    Jump hole = jit.branchTest64(SpecializedThunkJIT::NonZero, SpecializedThunkJIT::regT0);

Isn't this case "notHole"?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1055
> +    Jump hole = jit.branch32(SpecializedThunkJIT::NotEqual, SpecializedThunkJIT::regT3, TrustedImm32(JSValue::EmptyValueTag));

Ditto.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1083
> +    jit.appendFailure(jit.branch32(SpecializedThunkJIT::NotEqual, SpecializedThunkJIT::regT3, TrustedImm32(DoubleShape)));
> +
> +    jit.loadPtr(Address(SpecializedThunkJIT::regT0, JSObject::butterflyOffset()), SpecializedThunkJIT::regT2);
> +
> +    jit.loadDouble(BaseIndex(SpecializedThunkJIT::regT2, SpecializedThunkJIT::regT1, SpecializedThunkJIT::TimesEight), SpecializedThunkJIT::fpRegT0);
> +    
> +    jit.add32(TrustedImm32(1), Address(SpecializedThunkJIT::regT4, JSArrayIterator::offsetOfNextIndex()));
> +    
> +    jit.returnDouble(SpecializedThunkJIT::fpRegT0);

Why are there newlines between each of these lines?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:316
> -
> +    

Revert.

> LayoutTests/js/script-tests/array-iterators.js:77
> +

It would be nice to test each of the different paths in the JIT code (i.e. int32s, doubles, undecided/values). I think I see int32s and values here already. Are we sure that these tests run long enough to get into the baseline JIT?

[Oliver Hunt]

Created attachment 213930 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 213930 [details]
Patch

r=me with changes we discussed.

[Oliver Hunt]

Committed r157267: <http://trac.webkit.org/changeset/157267>