Bug 122575

Created attachment 213817 [details] Patch

Created attachment 213821 [details] Patch

You added whitespace in Source/JavaScriptCore/runtime/JSGlobalObject.cpp Can you please document arrayIteratorNextThunkGenerator(VM* vm) ?

Created attachment 213926 [details] Patch

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?

Created attachment 213930 [details] Patch

Comment on attachment 213930 [details] Patch r=me with changes we discussed.

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