| Summary: | Redesign how we do for-of iteration for JSArrays | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||||||||||||||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Keith Miller <keith_miller> | ||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | aakash_jain, annulen, aperez, ap, benjamin, cdumez, cgarcia, clopez, cmarcelo, ddkilzer, don.olmstead, ews-watchlist, fpizlo, gyuyoung.kim, Hironori.Fujii, jmason, joepeck, mark.lam, msaboff, ross.kirsling, ryuan.choi, saam, sam, sergio, simon.fraser, tzagallo, webkit-bot-watchers-bugzilla, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=210718 https://bugs.webkit.org/show_bug.cgi?id=210721 https://bugs.webkit.org/show_bug.cgi?id=211095 |
||||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 210023 | ||||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||
|
Description
Keith Miller
2017-08-10 15:58:57 PDT
We do? We do! Created attachment 317898 [details]
WIP works in LLInt
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);
Created attachment 396501 [details]
Patch
Created attachment 396503 [details]
WIP
Created attachment 396507 [details]
WIP
Created attachment 396509 [details]
WIP
Created attachment 396510 [details]
WIP
Created attachment 396511 [details]
WIP
Created attachment 396534 [details]
WIP
The title is very generic. Can you make it a little more JSC-specific? Created attachment 396555 [details]
Patch
Created attachment 396557 [details]
Patch
Created attachment 396558 [details]
Patch
Created attachment 396562 [details]
Patch
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? 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. 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. Created attachment 396608 [details]
Patch
Created attachment 396609 [details]
Patch
Created attachment 396611 [details]
Patch
Created attachment 396613 [details]
Patch
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. Created attachment 396690 [details]
Patch
Created attachment 396692 [details]
Patch
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. 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. Created attachment 396731 [details]
Patch for landing
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 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 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? 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. (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. (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 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? (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. 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 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 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? Comment on attachment 396731 [details]
Patch for landing
r=me too
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? 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. Created attachment 396863 [details]
Patch for landing
Committed r260323: <https://trac.webkit.org/changeset/260323> 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> (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? (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. Oh man, this toasted WinCairo. Just entering `for (let x of [1,2,3]) print(x);` suffices to crash the REPL. AppleWin port is also crashing. https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Tests%29/builds/5767 > 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 (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 (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. |