Bug 122575 - Further improve ArrayIterator performance
Summary: Further improve ArrayIterator performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-09 14:35 PDT by Oliver Hunt
Modified: 2013-10-10 16:14 PDT (History)
1 user (show)

See Also:


Attachments
Patch (18.02 KB, patch)
2013-10-09 14:39 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (18.97 KB, patch)
2013-10-09 15:16 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (19.71 KB, patch)
2013-10-10 13:55 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (20.38 KB, patch)
2013-10-10 14:56 PDT, Oliver Hunt
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2013-10-09 14:35:44 PDT
Further improve ArrayIterator performance
Comment 1 Oliver Hunt 2013-10-09 14:39:45 PDT
Created attachment 213817 [details]
Patch
Comment 2 Oliver Hunt 2013-10-09 15:16:54 PDT
Created attachment 213821 [details]
Patch
Comment 3 Nadav Rotem 2013-10-09 18:06:17 PDT
You added whitespace in  Source/JavaScriptCore/runtime/JSGlobalObject.cpp

Can you please document arrayIteratorNextThunkGenerator(VM* vm)  ?
Comment 4 Oliver Hunt 2013-10-10 13:55:11 PDT
Created attachment 213926 [details]
Patch
Comment 5 Mark Hahnenberg 2013-10-10 14:15:40 PDT
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?
Comment 6 Oliver Hunt 2013-10-10 14:56:44 PDT
Created attachment 213930 [details]
Patch
Comment 7 Mark Hahnenberg 2013-10-10 16:06:25 PDT
Comment on attachment 213930 [details]
Patch

r=me with changes we discussed.
Comment 8 Oliver Hunt 2013-10-10 16:14:34 PDT
Committed r157267: <http://trac.webkit.org/changeset/157267>