Bug 175454 – Redesign how we do for-of iteration for JSArrays

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Keith Miller]

Currently, we allocate the iteration object most of the time. Even though we object allocation sink it in the FTL, we could do better everywhere else. My idea is to exploit the fact that the iterator is not semantically visible other than the call to the next function. As long as the next function does not change on each of the iterator prototypes, we should not need to materialize the iterator object.

[Simon Fraser (smfr)]

We do?

[Sam Weinig]

We do!

[Keith Miller]

Created attachment 317898 [details]
WIP works in LLInt

[Keith Miller]

Looks like this weaksauce implementation is already about 5x faster (in the llint) on:

let array = new Array(10000);                                                                                                                                                
array.fill(1);                                                                                                                                                               
                                                                                                                                                                             
let start = preciseTime();                                                                                                                                                   
for (let run = 0; run < 10000; run++) {                                                                                                                                      
    for (let i of array) { }                                                                                                                                                 
}                                                                                                                                                                            
let end = preciseTime();                                                                                                                                                     
                                                                                                                                                                             
print(end - start);

[Keith Miller]

Created attachment 396501 [details]
Patch

[Keith Miller]

Created attachment 396503 [details]
WIP

[Keith Miller]

Created attachment 396507 [details]
WIP

[Keith Miller]

Created attachment 396509 [details]
WIP

[Keith Miller]

Created attachment 396510 [details]
WIP

[Keith Miller]

Created attachment 396511 [details]
WIP

[Keith Miller]

Created attachment 396534 [details]
WIP

[Simon Fraser (smfr)]

The title is very generic. Can you make it a little more JSC-specific?

[Keith Miller]

Created attachment 396555 [details]
Patch

[Keith Miller]

Created attachment 396557 [details]
Patch

[Keith Miller]

Created attachment 396558 [details]
Patch

[Keith Miller]

Created attachment 396562 [details]
Patch

[Saam Barati]

Comment on attachment 396562 [details]
Patch

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

will continue to review in a few minutes

> Source/JavaScriptCore/ChangeLog:45
> +        The trickiest part of this patch was getting the hand rolled DFG
> +        IR to work correctly. This is because we don't have a great way to
> +        express large chucks of DFG graph that doesn't involve manually
> +        tracking all the DFG's invariants. Such as, Flushing/Phantoming
> +        values at the end of each block.

enumerate some of the other challenges?

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:44
> +OTHER_CFLAGS = $(inherited) -fno-slp-vectorize --system-header-prefix=unicode/;
> +OTHER_CPLUSPLUSFLAGS = $(inherited) -fno-slp-vectorize --system-header-prefix=unicode/;

this shouldn't be part of this change

> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:41
> +
> +    // Insert per metadata overrides here when we need them.
> +    return nullptr;

this is really weird

> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:60
> +    UNUSED_PARAM(checkpointIndex);

lol. A downside of "if constexpr" I suppose

> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:67
> +

nit: remove newline

> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:93
> +Operand destinationFor(const Bytecode& bytecode, unsigned checkpointIndex, JITType type = JITType::None)

feels like a weird use of JITType. (E.g, HostCallThunk, None)

> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:108
> +            return bytecode.m_value; // We reuse value as a temp because its either not used in subsequent bytecodes or written as the temp object is killed.

its => it's

I don't understand the second half of this sentence: "written as the temp object is killed."

> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:132
> +    if constexpr (Bytecode::opcodeID == op_iterator_open) {
> +        ASSERT(checkpointIndex == OpIteratorOpen::symbolCall);
> +        return bytecode.m_symbolIterator;
> +    } else if constexpr (Bytecode::opcodeID == op_iterator_next) {
> +        ASSERT(checkpointIndex == OpIteratorNext::computeNext);
> +        return bytecode.m_next;
> +    } else
> +        return bytecode.m_callee;

for this entire file, I think webkit style would say this should be:

if constexpr(...) return...
if constexpr(...) return...
return...

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:101
> +

revert

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1268
> +        m_metadata->forEach<OpIteratorOpen>([&] (auto& metadata) {
> +            if (metadata.m_modeMetadata.mode != GetByIdMode::Default)
> +                return;
> +            StructureID oldStructureID = metadata.m_modeMetadata.defaultMode.structureID;
> +            if (!oldStructureID || vm.heap.isMarked(vm.heap.structureIDTable().get(oldStructureID)))
> +                return;
> +            if (Options::verboseOSR())
> +                dataLogF("Clearing iterator open LLInt property access.\n");
> +            LLIntPrototypeLoadAdaptiveStructureWatchpoint::clearLLIntGetByIdCache(metadata.m_modeMetadata);
> +        });
> +
> +
> +        m_metadata->forEach<OpIteratorNext>([&] (auto& metadata) {
> +            auto clearIfNeeded = [&] (GetByIdModeMetadata& modeMetadata) {
> +                if (modeMetadata.mode != GetByIdMode::Default)
> +                    return;
> +                StructureID oldStructureID = modeMetadata.defaultMode.structureID;
> +                if (!oldStructureID || vm.heap.isMarked(vm.heap.structureIDTable().get(oldStructureID)))
> +                    return;
> +                dataLogLnIf(Options::verboseOSR(), "Clearing iterator next LLInt property access.\n");
> +                LLIntPrototypeLoadAdaptiveStructureWatchpoint::clearLLIntGetByIdCache(modeMetadata);
> +            };
> +
> +            clearIfNeeded(metadata.m_doneModeMetadata);
> +            clearIfNeeded(metadata.m_valueModeMetadata);
> +        });

it seem like there is a lot of room to share code here and with the below OpGetById.

Cant you just pass in GetByIdModeMetadata& to some helper lambda?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1385
> +                // FIXME: We don't really want to clear both caches here but it's kinda annoying to figure out which one this is referring to...

could the watchpoint hold a pointer? Or a checkpoint index?

> Source/JavaScriptCore/bytecode/CodeBlockInlines.h:42
> +        m_metadata->forEach<__op>([&] (auto& metadata) { func(metadata.m_profile, false); });

revert

> Source/JavaScriptCore/bytecode/CodeBlockInlines.h:58
> +        m_metadata->forEach<OpIteratorOpen>([&] (auto& metadata) { 
> +            func(metadata.m_iterableProfile, false);
> +            func(metadata.m_iteratorProfile, false);
> +            func(metadata.m_nextProfile, false);
> +        });
> +
> +        m_metadata->forEach<OpIteratorNext>([&] (auto& metadata) {
> +            func(metadata.m_nextResultProfile, false);
> +            func(metadata.m_doneProfile, false);
> +            func(metadata.m_valueProfile, false);
> +        });

I wish we gave true and false names in this forEachValueProfile function

> Source/JavaScriptCore/bytecode/GetByStatus.cpp:110
> +        } else {
> +            if (metadata.m_valueModeMetadata.mode != GetByIdMode::Default)

nit: assert checkpoint index?

> Source/JavaScriptCore/bytecode/IterationModeMetadata.h:41
> +    uint8_t seenModes { 0 };
> +    static_assert(sizeof(decltype(seenModes)) == sizeof(IterationMode));

why not just use IterationMode?

> Source/JavaScriptCore/bytecode/SpeculatedType.h:123
> +inline bool isSubtypeSpeculation(SpeculatedType value, SpeculatedType category)

maybe not for this patch, but it might be worth writing the other things as a call to this

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4573
>  

what AST pattern is this?
[a,b] = <expr>?

Is it doing the old thing?

> Source/JavaScriptCore/runtime/OptionsList.h:122
> +    v(Bool, useIterationIntrinsics, true, Normal, nullptr) \

explanation?

[Don Olmstead]

With regards to the build breakage on Windows where you're seeing the headers from JSCBuiltins.h not being copied this seems like it stems from https://bugs.webkit.org/show_bug.cgi?id=207119 which added a custom target JSCBuiltins. This isn't a dependency of JavaScriptCore_CopyPrivateHeaders so that explains why its not working.

Will take a look at it for you.

[Keith Miller]

Comment on attachment 396562 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:45
>> +        values at the end of each block.
> 
> enumerate some of the other challenges?

Sure.

>> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:44
>> +OTHER_CPLUSPLUSFLAGS = $(inherited) -fno-slp-vectorize --system-header-prefix=unicode/;
> 
> this shouldn't be part of this change

I moved it out.

>> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:41
>> +    return nullptr;
> 
> this is really weird

Fixed.

>> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:93
>> +Operand destinationFor(const Bytecode& bytecode, unsigned checkpointIndex, JITType type = JITType::None)
> 
> feels like a weird use of JITType. (E.g, HostCallThunk, None)

I didn't have a better thing to use. I'm open to suggestions though.

>> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:108
>> +            return bytecode.m_value; // We reuse value as a temp because its either not used in subsequent bytecodes or written as the temp object is killed.
> 
> its => it's
> 
> I don't understand the second half of this sentence: "written as the temp object is killed."

Uhh, I guess I was trying to say setting m_value is the last time we need the temp.

>> Source/JavaScriptCore/bytecode/BytecodeProfileFor.h:132
>> +        return bytecode.m_callee;
> 
> for this entire file, I think webkit style would say this should be:
> 
> if constexpr(...) return...
> if constexpr(...) return...
> return...

That doesn't work because you need the else, at least for the last statement, otherwise you'll get a compile error.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1268
>> +        });
> 
> it seem like there is a lot of room to share code here and with the below OpGetById.
> 
> Cant you just pass in GetByIdModeMetadata& to some helper lambda?

Good idea, I hoisted clearIfNeeded and split it among the cases.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1385
>> +                // FIXME: We don't really want to clear both caches here but it's kinda annoying to figure out which one this is referring to...
> 
> could the watchpoint hold a pointer? Or a checkpoint index?

Yeah, I just didn't think it was important enough to worry about here.

>> Source/JavaScriptCore/bytecode/CodeBlockInlines.h:42
>> +        m_metadata->forEach<__op>([&] (auto& metadata) { func(metadata.m_profile, false); });
> 
> revert

This was intentional because that's the indentation of the if.

>> Source/JavaScriptCore/bytecode/CodeBlockInlines.h:58
>> +        });
> 
> I wish we gave true and false names in this forEachValueProfile function

I'll file a bug.

>> Source/JavaScriptCore/bytecode/GetByStatus.cpp:110
>> +            if (metadata.m_valueModeMetadata.mode != GetByIdMode::Default)
> 
> nit: assert checkpoint index?

Sure.

>> Source/JavaScriptCore/bytecode/SpeculatedType.h:123
>> +inline bool isSubtypeSpeculation(SpeculatedType value, SpeculatedType category)
> 
> maybe not for this patch, but it might be worth writing the other things as a call to this

Sure, I just found this easier to understand when I was looking at the code. I'll do a more comprehensive refactor later.

[Keith Miller]

Created attachment 396608 [details]
Patch

[Keith Miller]

Created attachment 396609 [details]
Patch

[Keith Miller]

Created attachment 396611 [details]
Patch

[Keith Miller]

Created attachment 396613 [details]
Patch

[Keith Miller]

Comment on attachment 396613 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:74
> +        * bytecode/BytecodeProfileFor.h: Added.
> +        (JSC::arrayProfileForImpl):
> +        (JSC::hasArrayProfileFor):
> +        (JSC::arrayProfileFor):
> +        (JSC::valueProfileForImpl):
> +        (JSC::hasValueProfileFor):
> +        (JSC::valueProfileFor):
> +        (JSC::destinationFor):
> +        (JSC::calleeFor):
> +        (JSC::argumentCountIncludingThisFor):
> +        (JSC::stackOffsetInRegistersForCall):
> +        (JSC::callLinkInfoFor):

I'm gonna rename this file BytecodeOperandsForCheckpoint.h. Open to other names though.

[Keith Miller]

Created attachment 396690 [details]
Patch

[Keith Miller]

Created attachment 396692 [details]
Patch

[Filip Pizlo]

Comment on attachment 396692 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:973
> +    FrozenValue* bottomValueMatchingSpeculation(SpeculatedType spec)

I feel like this helper should be in Graph.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6750
> +                    flushForTerminal();

You don't need this for branches unless they can go backward.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6836
> +                    flushForTerminal();

You don't have to do this unless this is a backward jump.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6854
> +                flushForTerminal();

You don't have to do this unless this is a backward jump.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6913
> +                    flushForTerminal();

You don't have to do this unless one of the destinations is backwards.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6935
> +                    flushForTerminal();

You don't have to do this unless one of the destinations is backwards.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6965
> +                    flushForTerminal();

You don't have to do this unless the destination is backwards.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6989
> +                    flushForTerminal();

You don't have to do this unless the destination is backwards.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7019
> +                    flushForTerminal();

You don't have to do this unless one of the destinations is backwards.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7062
> +                    flushForTerminal();

You don't have to do this unless one of the destinations is backwards.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7085
> +                    flushForTerminal();

You don't have to do this unless the destination is backwards.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7104
> +                flushForTerminal();

You don't have to do this unless the destination is backwards.

[Keith Miller]

Comment on attachment 396692 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:973
>> +    FrozenValue* bottomValueMatchingSpeculation(SpeculatedType spec)
> 
> I feel like this helper should be in Graph.

Fair enough, I'll move it.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6750
>> +                    flushForTerminal();
> 
> You don't need this for branches unless they can go backward.

Got it. We should have a comment on flushForTerminal that clarifies it's only needed for Back edges / returns / sideExit terminals.

[Keith Miller]

Created attachment 396731 [details]
Patch for landing

[EWS]

Found 30 new test failures: imported/w3c/web-platform-tests/IndexedDB/idlharness.any.html, http/wpt/entries-api/interfaces.html, imported/w3c/web-platform-tests/domparsing/idlharness.window.html, imported/w3c/web-platform-tests/css/css-fonts/idlharness.html, imported/w3c/web-platform-tests/fetch/api/abort/cache.https.html, imported/w3c/web-platform-tests/css/cssom-view/idlharness.html, imported/w3c/web-platform-tests/css/geometry/interfaces.html, imported/w3c/web-platform-tests/WebCryptoAPI/idlharness.https.html, imported/w3c/web-platform-tests/hr-time/idlharness.html, imported/w3c/web-platform-tests/css/css-properties-values-api/idlharness.html, imported/w3c/web-platform-tests/encoding/idlharness.any.html, imported/w3c/web-platform-tests/IndexedDB/idlharness.any.worker.html, imported/w3c/web-platform-tests/css/geometry/interfaces.worker.html, imported/w3c/web-platform-tests/css/css-font-loading/idlharness.https.html, imported/w3c/web-platform-tests/credential-management/idlharness.https.window.html, imported/w3c/web-platform-tests/FileAPI/idlharness.worker.html, imported/w3c/web-platform-tests/dom/idlharness.window.html, imported/w3c/web-platform-tests/css/css-animations/idlharness.html, imported/w3c/web-platform-tests/fetch/api/idl.any.worker.html, imported/w3c/web-platform-tests/encoding/idlharness.any.worker.html, imported/w3c/web-platform-tests/css/css-fonts/font-display/font-display-feature-policy-reporting.tentative.html, imported/w3c/web-platform-tests/content-security-policy/securitypolicyviolation/idlharness.window.html, http/wpt/credential-management/idl.https.html, http/tests/uri/curly-braces-escaping.html, imported/w3c/web-platform-tests/FileAPI/idlharness.html, http/wpt/fetch/response-opaque-clone.html, imported/w3c/web-platform-tests/dom/idlharness.any.worker.html, imported/w3c/web-platform-tests/WebIDL/interfaces.html, imported/w3c/web-platform-tests/css/css-transitions/idlharness.html, imported/w3c/web-platform-tests/fetch/api/idl.any.html

[Saam Barati]

Comment on attachment 396731 [details]
Patch for landing

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6681
> +            unsigned numberOfRemainingModes = WTF::bitCount(seenModes);

this should be called "numberOfSeenModes"

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6706
> +                    Node* knownIterFunction = addToGraph(CompareEqPtr, OpInfo(frozenSymbolIteratorFunction), symbolIterator);

nit: knownIterFunction => isKnownIterFunction"

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6720
> +                    flushForTerminal();

this shouldn't be needed based on what we talked about in Slack w/ Phil, right?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1678
> +//    dataLogLn("Calling slow open_call");

oops

[Saam Barati]

Comment on attachment 396731 [details]
Patch for landing

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6717
> +                    m_exitOK = true;
> +                    addToGraph(ExitOK);

What clobbered exit state?

[Keith Miller]

Comment on attachment 396731 [details]
Patch for landing

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6717
>> +                    addToGraph(ExitOK);
> 
> What clobbered exit state?

ArithBitAnd clobbers exit state for some reason...

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6720
>> +                    flushForTerminal();
> 
> this shouldn't be needed based on what we talked about in Slack w/ Phil, right?

Yeah. you're right I think I missed it.

[Filip Pizlo]

(In reply to Keith Miller from comment #33)
> Comment on attachment 396731 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396731&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6717
> >> +                    addToGraph(ExitOK);
> > 
> > What clobbered exit state?
> 
> ArithBitAnd clobbers exit state for some reason...

Because it could be effectful if not inferred you be effect free. 

I would assume it can’t have effects in this case. 

Or can it?

> 
> >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6720
> >> +                    flushForTerminal();
> > 
> > this shouldn't be needed based on what we talked about in Slack w/ Phil, right?
> 
> Yeah. you're right I think I missed it.

[Keith Miller]

(In reply to Filip Pizlo from comment #34)
> (In reply to Keith Miller from comment #33)
> > Comment on attachment 396731 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=396731&action=review
> > 
> > >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6717
> > >> +                    addToGraph(ExitOK);
> > > 
> > > What clobbered exit state?
> > 
> > ArithBitAnd clobbers exit state for some reason...
> 
> Because it could be effectful if not inferred you be effect free. 
> 
> I would assume it can’t have effects in this case. 
> 
> Or can it?
> 

In this case it's bit anding two JSBooleans. So it shouldn't.

[Saam Barati]

(In reply to Keith Miller from comment #35)
> (In reply to Filip Pizlo from comment #34)
> > (In reply to Keith Miller from comment #33)
> > > Comment on attachment 396731 [details]
> > > Patch for landing
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=396731&action=review
> > > 
> > > >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6717
> > > >> +                    addToGraph(ExitOK);
> > > > 
> > > > What clobbered exit state?
> > > 
> > > ArithBitAnd clobbers exit state for some reason...
> > 
> > Because it could be effectful if not inferred you be effect free. 
> > 
> > I would assume it can’t have effects in this case. 
> > 
> > Or can it?
> > 
> 
> In this case it's bit anding two JSBooleans. So it shouldn't.

In the spirit of the other explicit drops of ExitOK node, can you add a comment?

[Saam Barati]

(In reply to Saam Barati from comment #36)
> (In reply to Keith Miller from comment #35)
> > (In reply to Filip Pizlo from comment #34)
> > > (In reply to Keith Miller from comment #33)
> > > > Comment on attachment 396731 [details]
> > > > Patch for landing
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=396731&action=review
> > > > 
> > > > >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6717
> > > > >> +                    addToGraph(ExitOK);
> > > > > 
> > > > > What clobbered exit state?
> > > > 
> > > > ArithBitAnd clobbers exit state for some reason...
> > > 
> > > Because it could be effectful if not inferred you be effect free. 
> > > 
> > > I would assume it can’t have effects in this case. 
> > > 
> > > Or can it?
> > > 
> > 
> > In this case it's bit anding two JSBooleans. So it shouldn't.
> 
> In the spirit of the other explicit drops of ExitOK node, can you add a
> comment?

Also, if you provide UseKinds, I bet the clobbering of exit state goes away.

[Saam Barati]

Comment on attachment 396731 [details]
Patch for landing

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6715
> +                    // We know the bit and cannot have effects so it's ok to exit here.

"bit and" => "ArithBitAnd"

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6735
> +                set(bytecode.m_next, jsConstant(JSValue()));

nit: Might be worth a comment somewhere that empty for m_next means fast (if you don't already have such a comment)

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6737
> +                // Do our set locals. We don't want to run our getByVal again so we move to the next bytecode.

what getByVal you talking about?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6740
> +                addToGraph(ExitOK);

I don't think this is needed

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6767
> +                addToGraph(Branch, OpInfo(branchData), addToGraph(IsObject, iterator));

feels like we should have a value profile for the call so this can be a speculation sometimes

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6823
> +                flushForTerminal();

not needed

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6864
> +                }

seems like you need an
```
else
    addToGraph(CheckIsEmpty, m_next)
```
here

[Saam Barati]

Comment on attachment 396731 [details]
Patch for landing

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6867
> +                addToGraph(CheckStructure, OpInfo(m_graph.addStructureSet(globalObject->arrayIteratorStructure())), iterator);

do we have a way of backing off on this? Or could one do an OSR exit loop here?

I'm worried about the numberOfRemainingModes > 1, and we do have the fast array iterator function, but just not this structure

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6895
> +                    Node* isDone = addToGraph(CompareGreaterEq, index, length);

you probably can provide UseKinds here I'm guessing

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1546
> +    if (speculationContains(prediction, SpecString | SpecSymbol))
> +        return freeze(m_vm.smallStrings.emptyString());

can we make this handle symbol properly?

This will pessimize an array containing symbols.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1549
> +    if (speculationContains(prediction, SpecCellOther | SpecObject))
> +        return freeze(jsNull());

this feels like it falls really short. Some quick glancing at FixupPhase, and you could make a lot of inner loops slower. Some things I glanced at that I think might get worse:

```
StringObjectUse
StringOrStringObjectUse


RegExpExec
RegExpTest
StringReplace
StringReplaceRegExp
GetPrototypeOf
Spread
ToPropertyKey

any node that asks isFinalObjectSpeculation

(... and probably there is more)
```

I think we need to prove to ourselves that some of this indeed gets worse. And we need to have a strategy about what to do here.

> Source/JavaScriptCore/dfg/DFGLazyJSValue.h:80
> +    SpeculatedType speculatedType() const { return kind() == KnownValue ? SpecBytecodeTop : SpecString; }

can we do better? I know this isn't used for anything but strings right now, but if we start using it for more, SpecBytecodeTop will be a footgun

Or perhaps just assert for now?

> Source/JavaScriptCore/dfg/DFGOSRExit.h:162
> +    unsigned m_dfgIRIndex;

delete

[Saam Barati]

Comment on attachment 396731 [details]
Patch for landing

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

> Source/JavaScriptCore/ChangeLog:23
> +        `nextResult = next.@call(iterator); done = nextResult.done; value
> +        = done ? undefined : nextResult.value`, where nextResult is a

nit: put the code on one line or somehow structured better. It's confusing as written now

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6700
> +                if (numberOfRemainingModes == 1)

nit: It feels like the appropriate check here and elsewhere in this file is " == FastArray" so when we emit other cases later it's obvious what's going on.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6928
> +                    addToGraph(ExitOK);

not needed

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6965
> +                    m_currentBlock = genericBlock;
> +                    clearCaches();

you do this a lot. I wonder if we should have a helper that sets m_currentBlock and calls clearCaches()

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7018
> +                    // Set a value for m_value so we don't think

incomplete thought

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7025
> +                    addToGraph(Branch, OpInfo(branchData), get(bytecode.m_done));

if we exit here do we do the right thing? Might be good for a test

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:2115
> +    else
> +        valueRegister = iteratorResultObject.get(globalObject, vm.propertyNames->value);

do we have tests for an exception here along OSR exit path?

[Saam Barati]

Comment on attachment 396731 [details]
Patch for landing

r=me too

[Saam Barati]

Comment on attachment 396731 [details]
Patch for landing

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

Do you have tests for using the array iterator with a non array |this|?

> Source/JavaScriptCore/jit/JIT.cpp:264
> +            dataLogLn("Baseline JIT emitting code for ", m_bytecodeIndex, " at offset ", (long)debugOffset());

lol

> Source/JavaScriptCore/jit/JIT.h:300
> +        void checkpointReached();

why is this only called for iterators but not varargs calls?

> Source/JavaScriptCore/jit/JITCall.cpp:396
> +    auto* tryFastFunction = ([&] () {
> +        switch (instruction->width()) {
> +        case Narrow: return iterator_open_try_fast_narrow;
> +        case Wide16: return iterator_open_try_fast_wide16;
> +        case Wide32: return iterator_open_try_fast_wide32;
> +        default: RELEASE_ASSERT_NOT_REACHED();
> +        }
> +    })();

is it worth a FIXME to do this more generally?

Is it still a speedup?

You also do it below, so maybe factor it out into a tiny helper that takes instruction?

> Source/JavaScriptCore/jit/JITInlines.h:110
> +    uint32_t locationBits = CallSiteIndex(m_bytecodeIndex.offset()).bits();

Is the invariant we just never store checkpoint index into the call frame header?

> Source/JavaScriptCore/jit/JITInlines.h:188
> +inline void JIT::checkpointReached()

nit: maybe "advanceToNextCheckpoint" instead?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1689
> +//    dataLogLn("Calling slow next_call");

oops

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:995
> +        // FIXME: This only works for arrays from the same global object as ourselves but we should be able to support any pairing.

can you make a bug? Seems like a good thing to do

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:999
> +        // We should be good to go.

"should be" => "are"

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1000
> +        metadata.m_iterationMetadata.seenModes = metadata.m_iterationMetadata.seenModes | IterationMode::FastArray;

style nit: |= ?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1005
> +        {
> +            iterator = JSArrayIterator::create(vm, globalObject->arrayIteratorStructure(), iteratedObject, IterationKind::Values);
> +        }

why is this in a block?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1016
> +    metadata.m_iterationMetadata.seenModes = metadata.m_iterationMetadata.seenModes | IterationMode::Generic;

ditto |= ?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1056
> +            bool done = index == -1 || index >= array->length();

can we define "-1" as a constant somewhere since it's hard coded in a few places?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1061
> +                ASSERT(index == static_cast<unsigned>(index));

we assert here, but do an overflow check in DFG. Seems like a bad inconsistency. We should decide if it's possible or not

> Source/JavaScriptCore/runtime/CommonSlowPaths.h:250
> +// SLOW_PATH_HIDDEN_DECL(iterator_open_try_fast);
> +// SLOW_PATH_HIDDEN_DECL(iterator_next_try_fast);

?

> Source/JavaScriptCore/runtime/JSCast.h:69
> +    macro(JSArrayIterator, JSType::JSArrayIteratorType, JSType::JSArrayIteratorType) \

why this change?

[Keith Miller]

Comment on attachment 396731 [details]
Patch for landing

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6767
>> +                addToGraph(Branch, OpInfo(branchData), addToGraph(IsObject, iterator));
> 
> feels like we should have a value profile for the call so this can be a speculation sometimes

There is a value profile for iterator. It comes from the m_iteratorProfile that gets filled in the LLInt/Baseline.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6864
>> +                }
> 
> seems like you need an
> ```
> else
>     addToGraph(CheckIsEmpty, m_next)
> ```
> here

We don't have a  CheckIsEmpty node only CheckNotEmpty. I'm going to change CheckCell into CheckIsConstant that equivalent to JSValue::operator==().

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6867
>> +                addToGraph(CheckStructure, OpInfo(m_graph.addStructureSet(globalObject->arrayIteratorStructure())), iterator);
> 
> do we have a way of backing off on this? Or could one do an OSR exit loop here?
> 
> I'm worried about the numberOfRemainingModes > 1, and we do have the fast array iterator function, but just not this structure

We currently only support this when our global object is the same as the symbolIterator of the object entering the loop. It's mostly just a sanity check for our GetInternalFields below. This check is should actually impossible to fire and should get eliminated by AI.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6965
>> +                    clearCaches();
> 
> you do this a lot. I wonder if we should have a helper that sets m_currentBlock and calls clearCaches()

Possibly, we may want a better way to do all of this... It's a nightmare to do by hand.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7018
>> +                    // Set a value for m_value so we don't think
> 
> incomplete thought

Fixed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7025
>> +                    addToGraph(Branch, OpInfo(branchData), get(bytecode.m_done));
> 
> if we exit here do we do the right thing? Might be good for a test

I have a test where we OSR when getting done. I'm not sure how to make a test that would exit at the branch though.

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1546
>> +        return freeze(m_vm.smallStrings.emptyString());
> 
> can we make this handle symbol properly?
> 
> This will pessimize an array containing symbols.

Per offline discussion, we should fix this in a different way. Which we will do in a follow up patch. https://bugs.webkit.org/show_bug.cgi?id=210694

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1549
>> +        return freeze(jsNull());
> 
> this feels like it falls really short. Some quick glancing at FixupPhase, and you could make a lot of inner loops slower. Some things I glanced at that I think might get worse:
> 
> ```
> StringObjectUse
> StringOrStringObjectUse
> 
> 
> RegExpExec
> RegExpTest
> StringReplace
> StringReplaceRegExp
> GetPrototypeOf
> Spread
> ToPropertyKey
> 
> any node that asks isFinalObjectSpeculation
> 
> (... and probably there is more)
> ```
> 
> I think we need to prove to ourselves that some of this indeed gets worse. And we need to have a strategy about what to do here.

Ditto. https://bugs.webkit.org/show_bug.cgi?id=210694

>> Source/JavaScriptCore/dfg/DFGLazyJSValue.h:80
>> +    SpeculatedType speculatedType() const { return kind() == KnownValue ? SpecBytecodeTop : SpecString; }
> 
> can we do better? I know this isn't used for anything but strings right now, but if we start using it for more, SpecBytecodeTop will be a footgun
> 
> Or perhaps just assert for now?

Not sure what assertion we should do here? Regardless, I feel like when we add new enum values it's kind of our responsibility to make sure all the members make sense with the new entry...

>> Source/JavaScriptCore/dfg/DFGOSRExit.h:162
>> +    unsigned m_dfgIRIndex;
> 
> delete

Done.

>> Source/JavaScriptCore/jit/JIT.h:300
>> +        void checkpointReached();
> 
> why is this only called for iterators but not varargs calls?

We only use it right now to nicely forward the checkpoint to BytecodeOperandsForCheckpoint.h functions. Varargs calls doesn't do that so it's not an issue.

>> Source/JavaScriptCore/jit/JITCall.cpp:396
>> +    })();
> 
> is it worth a FIXME to do this more generally?
> 
> Is it still a speedup?
> 
> You also do it below, so maybe factor it out into a tiny helper that takes instruction?

Yeah, it's still a important for perf.

do you mean like a macro or something? I'm not sure how it would work otherwise...

>> Source/JavaScriptCore/jit/JITInlines.h:110
>> +    uint32_t locationBits = CallSiteIndex(m_bytecodeIndex.offset()).bits();
> 
> Is the invariant we just never store checkpoint index into the call frame header?

Yeah, I have a bug to go back to the old way where we only store the offset. I originally did this because I thought we'd want to know the checkpoint in the LLInt/Baseline but it didn't work out that way...

>> Source/JavaScriptCore/jit/JITInlines.h:188
>> +inline void JIT::checkpointReached()
> 
> nit: maybe "advanceToNextCheckpoint" instead?

Sure. I also added an assert that the instruction we are generating actually has checkpoints.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:2115
>> +        valueRegister = iteratorResultObject.get(globalObject, vm.propertyNames->value);
> 
> do we have tests for an exception here along OSR exit path?

Yeah, I made a test where both done and value are a getters that OSR once tiered up.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:995
>> +        // FIXME: This only works for arrays from the same global object as ourselves but we should be able to support any pairing.
> 
> can you make a bug? Seems like a good thing to do

Done.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1000
>> +        metadata.m_iterationMetadata.seenModes = metadata.m_iterationMetadata.seenModes | IterationMode::FastArray;
> 
> style nit: |= ?

I wasn't able to get assignment overloading to work for enum classes... which is why I did it this way.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1005
>> +        }
> 
> why is this in a block?

Fixed.

>> Source/JavaScriptCore/runtime/JSCast.h:69
>> +    macro(JSArrayIterator, JSType::JSArrayIteratorType, JSType::JSArrayIteratorType) \
> 
> why this change?

It was just a nit change that I wanted to make.

[Keith Miller]

Created attachment 396863 [details]
Patch for landing

[Keith Miller]

Committed r260323: <https://trac.webkit.org/changeset/260323>

[Radar WebKit Bug Importer]

<rdar://problem/61984236>

[David Kilzer (:ddkilzer)]

This appears to have caused a build failure on the Apple Windows 10 buildbot:

List of builds:  <https://build.webkit.org/builders/Apple%20Win%2010%20Release%20(Build)?numbuilds=50>

Failing build log:  <https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/builds/12731/steps/compile-webkit/logs/stdio>

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #47)
> This appears to have caused a build failure on the Apple Windows 10 buildbot:
> 
> List of builds: 
> <https://build.webkit.org/builders/
> Apple%20Win%2010%20Release%20(Build)?numbuilds=50>
> 
> Failing build log: 
> <https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/
> builds/12731/steps/compile-webkit/logs/stdio>

Fixed in r260325?

[Keith Miller]

(In reply to David Kilzer (:ddkilzer) from comment #48)
> (In reply to David Kilzer (:ddkilzer) from comment #47)
> > This appears to have caused a build failure on the Apple Windows 10 buildbot:
> > 
> > List of builds: 
> > <https://build.webkit.org/builders/
> > Apple%20Win%2010%20Release%20(Build)?numbuilds=50>
> > 
> > Failing build log: 
> > <https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/
> > builds/12731/steps/compile-webkit/logs/stdio>
> 
> Fixed in r260325?

The windows build is semi-broken for changes to derived sources... It's gonna take a clean build to fix.

[Ross Kirsling]

Oh man, this toasted WinCairo.

Just entering `for (let x of [1,2,3]) print(x);` suffices to crash the REPL.

[Fujii Hironori]

AppleWin port is also crashing. https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Tests%29/builds/5767

[Aakash Jain]

> Committed r260323: <https://trac.webkit.org/changeset/260323>
This seems to have introduced 8 JSC test failures on armv7/mips related to stress/for-of-array-different-globals.js
Test history: https://results.webkit.org/?suite=javascriptcore-tests&test=stress%2Ffor-of-array-different-globals.js.default

e.g.: https://ews-build.webkit.org/#/builders/25/builds/15247

[Jim Mason]

(In reply to Keith Miller from comment #45)
> Committed r260323: <https://trac.webkit.org/changeset/260323>

On Solaris x86_64, it is newly failing whilst building javascriptcoregtk-4.0.so:

Text relocation remains                           referenced
    against symbol                  offset        in file
iterator_next_try_fast_narrow       0x1156b       CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
iterator_next_try_fast_wide16       0x11a49       CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
iterator_next_try_fast_wide32       0x11f28       CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
iterator_open_try_fast_narrow       0x10b9f       CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
iterator_open_try_fast_wide32       0x11223       CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
iterator_open_try_fast_wide16       0x10ee0       CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
ld: fatal: relocations remain against allocatable but non-writable sections

[Jim Mason]

(In reply to Jim Mason from comment #53)
> (In reply to Keith Miller from comment #45)
> > Committed r260323: <https://trac.webkit.org/changeset/260323>
> 
> On Solaris x86_64, it is newly failing whilst building
> javascriptcoregtk-4.0.so:
> 
> Text relocation remains                           referenced
>     against symbol                  offset        in file
> iterator_next_try_fast_narrow       0x1156b      
> CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
> iterator_next_try_fast_wide16       0x11a49      
> CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
> iterator_next_try_fast_wide32       0x11f28      
> CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
> iterator_open_try_fast_narrow       0x10b9f      
> CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
> iterator_open_try_fast_wide32       0x11223      
> CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
> iterator_open_try_fast_wide16       0x10ee0      
> CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
> ld: fatal: relocations remain against allocatable but non-writable sections

This issue has cleared as of r260374.