Bug 205232

Created attachment 385671 [details] Patch

Created attachment 385672 [details] Patch

Created attachment 385674 [details] Patch

Created attachment 385676 [details] Patch

Created attachment 385677 [details] Patch

Created attachment 385679 [details] Patch

Created attachment 385680 [details] Patch

Comment on attachment 385680 [details] Patch Weird all the non-apply tests are passing locally for me... will investigate

Created attachment 386022 [details] Patch

Created attachment 386025 [details] Patch

Created attachment 386029 [details] Patch

Created attachment 386031 [details] Patch

Created attachment 386034 [details] Patch

> ../../Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:575:56: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare] > for (unsigned i = 0; i < inlineCallFrame->argumentCountIncludingThis - 1; ++i) { Wait what?? inlineCallFrame->argumentCountIncludingThis is unsigned so the result of `inlineCallFrame->argumentCountIncludingThis - 1` should also be unsigned! Ugh

Created attachment 386040 [details] Patch

Created attachment 386041 [details] Patch

Created attachment 386047 [details] Patch

Created attachment 386053 [details] Patch

Created attachment 386054 [details] Patch

Created attachment 386056 [details] Patch

Created attachment 386057 [details] Patch

Created attachment 386062 [details] Patch

Created attachment 386118 [details] Patch

Comment on attachment 386118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386118&action=review Wowza. I really like this patch. I'm gonna hold off on r+ until some of my comments/questions/concerns are addressed > Source/JavaScriptCore/ChangeLog:19 > + When the DFG OSR exits if there are any active checkpoints inline you should explain what a checkpoint is before this sentence. Might also be worth saying how you packed in checkpoint index into bytecode index > Source/JavaScriptCore/ChangeLog:34 > + refers to virtual registers as a int as possible. Instead "a int" => "an int" "Instead" => "Instead, " > Source/JavaScriptCore/ChangeLog:36 > + couple of new classes/enum added to JSC: "enum" => "enums" > Source/JavaScriptCore/ChangeLog:56 > + 3) FTL::SelectPredictability is a new enum that describes to the > + FTL whether or not we think a select is going to be > + predictable. SelectPredictability has four options: Unpredictable, > + Predictable, LeftLikely, and RightLikely. Unpredictable means we > + think a branch predictor won't do a good job guessing this value > + so we should compile the select to a cmov. The other options mean > + we either think we are going to pick the same value every time or > + there's a reasonable chance the branch predictor will be able to > + guess the value. this is a random point. Why does this patch include this? > Source/JavaScriptCore/ChangeLog:61 > + getters twice if we OSR exit during from LoadVarargs. "during from" => "during" or "from" > Source/JavaScriptCore/bytecode/BytecodeIndex.h:51 > + static constexpr uint32_t numberOfCheckpoints = 4; This should be called maxNumberOfCheckpoints > Source/JavaScriptCore/bytecode/FullBytecodeLiveness.h:37 > +// Note: Full bytecode liveness does not track any information about the liveness of temps. "temps" => "tmps" > Source/JavaScriptCore/bytecode/Operands.h:41 > +enum OperandKind { ArgumentOperand, LocalOperand, TmpOperand }; why not "enum class OperandKind {Argument, Local, Tmp}"? > Source/JavaScriptCore/bytecode/Operands.h:140 > - m_values.fill(initialValue, numArguments + numLocals); > + m_values.grow(numArguments + numLocals + numTmps); > + m_values.fill(initialValue); why this change? > Source/JavaScriptCore/bytecode/Operands.h:240 > + m_values[localIndex(oldNumLocals + i)] = ensuredValue; this seems like a semantic change and slightly sketchy you're not constructing a new one of each entry. I think you should keep this "= T()", and let the compiler optimize away > Source/JavaScriptCore/bytecode/Operands.h:244 > + void ensureTmps(size_t size, const T& ensuredValue = T()) same comment > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3233 > + ASSERT(m_codeBlock->hasCheckpoints()); we don't have check points for tail call forward varargs too? > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:861 > + > + style nit: only one newline > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:956 > + unsigned frameArgumentCount = static_cast<unsigned>(inlineCallFrame->argumentCountIncludingThis - 1); this is needed? > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:65 > + InsertionSet& insertionSet, Node* arguments, unsigned nodeIndex, NodeOrigin origin, bool addThis) style nit: This should be an enum, not bool, according to WK style > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:146 > + insertionSet.insertConstantForUse(nodeIndex, origin, jsNumber(addThis), Int32Use)); nit: jsNumber(static_cast<int32_t>(addThis)) so people don't go crazy reading this LOC > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:285 > + void progressCheckpoint() nit: progressCheckpoint => progressToNextCheckpoint? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:423 > + Node* getLocalOrTmp(Operand operand) maybe file a bug on a rename here? Should we name the node Get or GetOperand instead of keeping it GetLocal? (Same w/ SetLocal) > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:635 > + ASSERT(operand.isTmp() || !operand.virtualRegister().isConstant()); nit: maybe just add an isConstant() to Operand that is this logic? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1130 > + // The max number of temps used for forwarding data to an OSR exit to checkpoint. "to an OSR exit to checkpoint"? Did you mean to have the second "to"? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1662 > + ensureTmps((m_inlineStackTop->m_inlineCallFrame ? m_inlineStackTop->m_inlineCallFrame->tmpOffset : 0) + m_inlineStackTop->m_codeBlock->numTmps() + codeBlock->numTmps()); nit: Why not put this after the new inline stack entry constructor below, and just have it be: ensureTmps(m_inlineStackTop->m_inlineCallFrame->tmpOffset + codeBLock->numTmps()) > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1963 > + auto argCountTmp = m_inlineStackTop->remapOperand(Operand(TmpOperand, OpCallVarargs::argCount)); > + setDirect(argCountTmp, addToGraph(VarargsLength, OpInfo(data), arguments)); can we have a version that doesn't require us to call setDirect? Like a set that does remap for us? > Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp:497 > + RELEASE_ASSERT_NOT_REACHED(); is this needed? Wouldn't the switch be a compile error? > Source/JavaScriptCore/dfg/DFGClobberize.h:818 > + write(AbstractHeap(Stack, data->count)); I thought you said we don't do this anymore? > Source/JavaScriptCore/dfg/DFGCommonData.cpp:59 > + return DisposableCallSiteIndex(index); why all these DisposableCallSiteIndex changes? How is that right? > Source/JavaScriptCore/dfg/DFGForAllKills.h:78 > + if (after.bytecodeIndex().checkpoint()) { the code inside this "if" is assuming that these checkpoints belong to the same bytecode offset. Is that an ok assumption to make? Maybe assert that? I think this is an ok assumption, since when the DFG does code motion, it changes forExit origin, and this code is using forExit > Source/JavaScriptCore/dfg/DFGGraph.cpp:1222 > + if (verbose) > + dataLog("Ran out of stack, returning true.\n"); > + return true; > > RELEASE_ASSERT_NOT_REACHED(); what's going on here? Looks wrong... > Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:181 > + flush(Operand(TmpOperand, tmp)); nit: You use this constructor in a few places, would be nice to just have "Operand::tmp(tmp)" > Source/JavaScriptCore/dfg/DFGNodeType.h:77 > + /* These are used in SSA form to represent to track */\ track? > Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:246 > + case VarargsLength: { > + break; > + } why? > Source/JavaScriptCore/dfg/DFGOSRExit.cpp:641 > + addSideState(reinterpret_cast<CallFrame*>(reinterpret_cast<char*>(callFrame) + inlineCallFrame->returnPCOffset() - sizeof(CPURegister)), callBytecodeIndex, inlineCallFrame->tmpOffset); there isn't an offset just for call frame? > Source/JavaScriptCore/dfg/DFGOperations.cpp:2957 > + for (uint32_t i = length - 1; i < mandatoryMinimum; ++i) why - 1 now? > Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:524 > + RELEASE_ASSERT(node->op() == VarargsLength || node->op() == LoadVarargs || node->op() == ForwardVarargs); as I said in clobberize comment, does this actually write to that stack slot? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-7370 > - // https://bugs.webkit.org/show_bug.cgi?id=141448 you should dupe https://bugs.webkit.org/show_bug.cgi?id=141448 to this :-) > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7388 > + m_jit.store32(argumentCountIncludingThis, JITCompiler::payloadFor(data->machineCount)); seems like LoadVarargs should write to this. And VarargsLength should not. In clobberize > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9572 > + m_out.bitOr(m_out.isZero32(lengthIncludingThis), m_out.above(lengthIncludingThis, m_out.constInt32(data->limit)))); why would it be zero? DFG doesn't seem to do this? > Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:488 > + sideState->tmps[i] = tmpScratch[i + tmpOffset]; these are already boxed? This seems different than in the DFG, where you had to do value recovery. Is that right? Why not make the DFG also put the boxed value in the buffer before you read it out, so that DFG OSR exit code is cleaner? > Source/JavaScriptCore/interpreter/CallFrame.cpp:108 > + return CallSiteIndex::fromBits(unsafeCallSiteAsRawBits() & (std::numeric_limits<uint32_t>::max() >> BytecodeIndex::checkpointShift)); why not construct a BytecodeIndex and then call the helper function? This looks wrong anyways. I think you meant to mask off the checkpoint index. But that begs the question, why do you need to do that? That's wrong for DFG/FTL CallSiteIndices, right? Those are just indices into an array that we bound check in the sampling profiler. > Source/JavaScriptCore/interpreter/CallFrame.h:115 > + static constexpr int headerSizeInRegisters = static_cast<int>(CallFrameSlot::argumentCount) + 1; why is this static_cast needed if you overload the operators to work with CallFrameSlot? > Source/JavaScriptCore/interpreter/CheckpointOSRExitSideState.h:37 > + T getTmp(size_t offset) his is only used for JSValue. Let's just make this return JSValue. > Source/JavaScriptCore/interpreter/CheckpointOSRExitSideState.h:52 > + void* nextInstructionJumpTarget; this is unused > Source/JavaScriptCore/interpreter/Interpreter.cpp:545 > + // FIXME: We should support exception handling in checkpoints. hat do you mean by this? Please file a bug if it's important > Source/JavaScriptCore/jit/JITExceptions.cpp:53 > - CRASH(); > + WTFBreakpointTrap(); why? > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1969 > + unsigned argumentCountIncludingThis = sideState.getTmp<JSValue>(Opcode::argCount).asUInt32(); let's just make getTmp return JSValue Also, let's rename argCount to argCount to argCountIncludingThis for consistency > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1973 > + loadVarargs(globalObject, buffer, callFrame->r(bytecode.m_arguments).jsValue(), 0, argumentCountIncludingThis - 1); is zero the right offset here? Isn't it based on something in the opcode? > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1992 > +inline SlowPathReturnType dispatchNextInstruction(CodeBlock* codeBlock, InstructionStream::Ref pc) nit: I'd name this "dispatchToNextInstruction"? > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:2064 > + ASSERT_WITH_MESSAGE(pc.next()->opcodeID() == op_ret, "We strongly assume all tail calls are followed by an op_ret."); why does it really matter though? I don't see how it changes this code > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1780 > + > + nit: undo > Source/JavaScriptCore/runtime/VM.cpp:1031 > +void VM::addCheckpointOSRSideState(CallFrame* callFrame, std::unique_ptr<CheckpointOSRExitSideState>&& payload) Don't you need to visit these things?

Comment on attachment 386118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386118&action=review >> Source/JavaScriptCore/ChangeLog:19 >> + When the DFG OSR exits if there are any active checkpoints inline > > you should explain what a checkpoint is before this sentence. Might also be worth saying how you packed in checkpoint index into bytecode index True. I added a comment above. >> Source/JavaScriptCore/ChangeLog:34 >> + refers to virtual registers as a int as possible. Instead > > "a int" => "an int" > "Instead" => "Instead, " Fixed. >> Source/JavaScriptCore/ChangeLog:36 >> + couple of new classes/enum added to JSC: > > "enum" => "enums" Fixed. >> Source/JavaScriptCore/ChangeLog:61 >> + getters twice if we OSR exit during from LoadVarargs. > > "during from" => "during" or "from" Fixed. >> Source/JavaScriptCore/bytecode/Operands.h:41 >> +enum OperandKind { ArgumentOperand, LocalOperand, TmpOperand }; > > why not "enum class OperandKind {Argument, Local, Tmp}"? Sure! >> Source/JavaScriptCore/bytecode/Operands.h:140 >> + m_values.fill(initialValue); > > why this change? I think because FastBitVector didn't support it. I can make it work if you want but it probably doesn't matter for perf. >> Source/JavaScriptCore/bytecode/Operands.h:240 >> + m_values[localIndex(oldNumLocals + i)] = ensuredValue; > > this seems like a semantic change and slightly sketchy you're not constructing a new one of each entry. I think you should keep this "= T()", and let the compiler optimize away This only runs if ensuredValue != T(). And if it doesn't need initialization it presumably has a reasonable copy constructor. >> Source/JavaScriptCore/bytecode/Operands.h:244 >> + void ensureTmps(size_t size, const T& ensuredValue = T()) > > same comment Ditto. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3233 >> + ASSERT(m_codeBlock->hasCheckpoints()); > > we don't have check points for tail call forward varargs too? No, because when "forwarding varargsing" getting the length is not observable. >> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:956 >> + unsigned frameArgumentCount = static_cast<unsigned>(inlineCallFrame->argumentCountIncludingThis - 1); > > this is needed? Yeah, GCC seems to have a weird bug where it treats all bitfields as signed so the rhs becomes an int and we get an error. Although, this particular one might be unnecessary. >> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:65 >> + InsertionSet& insertionSet, Node* arguments, unsigned nodeIndex, NodeOrigin origin, bool addThis) > > style nit: This should be an enum, not bool, according to WK style It's not in the style guide! https://webkit.org/code-style-guidelines/ :P >> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:146 >> + insertionSet.insertConstantForUse(nodeIndex, origin, jsNumber(addThis), Int32Use)); > > nit: jsNumber(static_cast<int32_t>(addThis)) so people don't go crazy reading this LOC Sure. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:285 >> + void progressCheckpoint() > > nit: progressCheckpoint => progressToNextCheckpoint? Sure. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:423 >> + Node* getLocalOrTmp(Operand operand) > > maybe file a bug on a rename here? Should we name the node Get or GetOperand instead of keeping it GetLocal? (Same w/ SetLocal) Sure. https://bugs.webkit.org/show_bug.cgi?id=205489 >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:635 >> + ASSERT(operand.isTmp() || !operand.virtualRegister().isConstant()); > > nit: maybe just add an isConstant() to Operand that is this logic? Done. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1130 >> + // The max number of temps used for forwarding data to an OSR exit to checkpoint. > > "to an OSR exit to checkpoint"? Did you mean to have the second "to"? Fixed. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1963 >> + setDirect(argCountTmp, addToGraph(VarargsLength, OpInfo(data), arguments)); > > can we have a version that doesn't require us to call setDirect? Like a set that does remap for us? That seems reasonable but can we do it in a later patch? I'm not sure complicating this patch more is worthwhile. https://bugs.webkit.org/show_bug.cgi?id=205490 >> Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp:497 >> + RELEASE_ASSERT_NOT_REACHED(); > > is this needed? Wouldn't the switch be a compile error? Not on MSVC! >> Source/JavaScriptCore/dfg/DFGClobberize.h:818 >> + write(AbstractHeap(Stack, data->count)); > > I thought you said we don't do this anymore? Whoops, will remove. >> Source/JavaScriptCore/dfg/DFGCommonData.cpp:59 >> + return DisposableCallSiteIndex(index); > > why all these DisposableCallSiteIndex changes? How is that right? DisposableCallSiteIndex is what I'm using to refer to non-bytecode offset CallSites. Maybe that should be a different class from Disposable? >> Source/JavaScriptCore/dfg/DFGForAllKills.h:78 >> + if (after.bytecodeIndex().checkpoint()) { > > the code inside this "if" is assuming that these checkpoints belong to the same bytecode offset. Is that an ok assumption to make? Maybe assert that? > > I think this is an ok assumption, since when the DFG does code motion, it changes forExit origin, and this code is using forExit That was my thinking as well but an assert seems reasonable. >> Source/JavaScriptCore/dfg/DFGGraph.cpp:1222 >> RELEASE_ASSERT_NOT_REACHED(); > > what's going on here? Looks wrong... Whoops, I reorganized the loop but forgot to delete the now unreachable RELEASE_ASSERT_NOT_REACHED(). >> Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:181 >> + flush(Operand(TmpOperand, tmp)); > > nit: You use this constructor in a few places, would be nice to just have "Operand::tmp(tmp)" Fair enough. Added. >> Source/JavaScriptCore/dfg/DFGNodeType.h:77 >> + /* These are used in SSA form to represent to track */\ > > track? Whoops was going to add comments on what these did but I didn't get around to it. Will do in a follow up patch. >> Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:246 >> + } > > why? Because this doesn't set any side state. The tmp is set by the subsequent MovHint. >> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:641 >> + addSideState(reinterpret_cast<CallFrame*>(reinterpret_cast<char*>(callFrame) + inlineCallFrame->returnPCOffset() - sizeof(CPURegister)), callBytecodeIndex, inlineCallFrame->tmpOffset); > > there isn't an offset just for call frame? Not one that works here that I know of. There's CallFrame::returnPCOffset() but that doesn't handle the inline call frame offset. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:2957 >> + for (uint32_t i = length - 1; i < mandatoryMinimum; ++i) > > why - 1 now? Because we're handing the length + this now. I'll change the variable name. >> Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:524 >> + RELEASE_ASSERT(node->op() == VarargsLength || node->op() == LoadVarargs || node->op() == ForwardVarargs); > > as I said in clobberize comment, does this actually write to that stack slot? No, fixed. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9572 >> + m_out.bitOr(m_out.isZero32(lengthIncludingThis), m_out.above(lengthIncludingThis, m_out.constInt32(data->limit)))); > > why would it be zero? DFG doesn't seem to do this? Because VarargsLength could have overflowed so you'd wrap around to zero. Yeah, DFG does the same logic but just as two separate speculations. >> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:488 >> + sideState->tmps[i] = tmpScratch[i + tmpOffset]; > > these are already boxed? This seems different than in the DFG, where you had to do value recovery. Is that right? Why not make the DFG also put the boxed value in the buffer before you read it out, so that DFG OSR exit code is cleaner? Yeah, the FTL does it the right way. I'll add a FIXME to switch the DFG to using the same system as the FTL. The code could probably all be deduplicated anyway. >> Source/JavaScriptCore/interpreter/CallFrame.cpp:108 >> + return CallSiteIndex::fromBits(unsafeCallSiteAsRawBits() & (std::numeric_limits<uint32_t>::max() >> BytecodeIndex::checkpointShift)); > > why not construct a BytecodeIndex and then call the helper function? > > This looks wrong anyways. I think you meant to mask off the checkpoint index. But that begs the question, why do you need to do that? That's wrong for DFG/FTL CallSiteIndices, right? Those are just indices into an array that we bound check in the sampling profiler. Yeah, I think this will give the wrong line of code. I fixed it. I also think the OSR exit code will put the wrong bytecode index into the CallFrame so I fixed that too. >> Source/JavaScriptCore/interpreter/CheckpointOSRExitSideState.h:37 >> + T getTmp(size_t offset) > > his is only used for JSValue. Let's just make this return JSValue. Sure. >> Source/JavaScriptCore/interpreter/CheckpointOSRExitSideState.h:52 >> + void* nextInstructionJumpTarget; > > this is unused Removed. >> Source/JavaScriptCore/interpreter/Interpreter.cpp:545 >> + // FIXME: We should support exception handling in checkpoints. > > hat do you mean by this? > Please file a bug if it's important We don't support a catch block inside a single bytecode right now. We will probably have to do this for iteration_close in the future anyway. >> Source/JavaScriptCore/jit/JITExceptions.cpp:53 >> + WTFBreakpointTrap(); > > why? Because CRASH() doesn't do a breakpoint, it loads 0xbbadbeef... :/ >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1969 >> + unsigned argumentCountIncludingThis = sideState.getTmp<JSValue>(Opcode::argCount).asUInt32(); > > let's just make getTmp return JSValue > > Also, let's rename argCount to argCount to argCountIncludingThis for consistency Done. >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1973 >> + loadVarargs(globalObject, buffer, callFrame->r(bytecode.m_arguments).jsValue(), 0, argumentCountIncludingThis - 1); > > is zero the right offset here? Isn't it based on something in the opcode? Oh, yeah, that's right. I'll add a test where we do return foo.call(...args); in strict mode then OSR exit due to the length. >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1992 >> +inline SlowPathReturnType dispatchNextInstruction(CodeBlock* codeBlock, InstructionStream::Ref pc) > > nit: I'd name this "dispatchToNextInstruction"? Done. >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:2064 >> + ASSERT_WITH_MESSAGE(pc.next()->opcodeID() == op_ret, "We strongly assume all tail calls are followed by an op_ret."); > > why does it really matter though? I don't see how it changes this code It does matter because we dispatch to the next bytecode so if it's not a ret then we will be running whatever random bytecode is after the tail call. >> Source/JavaScriptCore/runtime/VM.cpp:1031 >> +void VM::addCheckpointOSRSideState(CallFrame* callFrame, std::unique_ptr<CheckpointOSRExitSideState>&& payload) > > Don't you need to visit these things? Yes. Added during Heap::gatherConservativeRoots()

Created attachment 386352 [details] Patch

Comment on attachment 386118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386118&action=review >>> Source/JavaScriptCore/dfg/DFGCommonData.cpp:59 >>> + return DisposableCallSiteIndex(index); >> >> why all these DisposableCallSiteIndex changes? How is that right? > > DisposableCallSiteIndex is what I'm using to refer to non-bytecode offset CallSites. Maybe that should be a different class from Disposable? Yeah this change looks wrong since callsiteindex is an index into a vector in DFG/FTL. Disposable is meant for things that go away, like ICs

Committed r253896: <https://trac.webkit.org/changeset/253896>

<rdar://problem/58171565>

Re-opened since this is blocked by bug 205582

Keith fixed the build in https://trac.webkit.org/r253903, we didn't roll back.

This patch unfortunately breaks the build on both arm and mips. The problem is related with a non-existing function: ../../Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:338:93: error: no matching function for call to ‘JSC::CodeBlock::instructions(JSC::BytecodeIndex&)’ const Instruction* instruction = jit.baselineCodeBlock()->instructions(bytecodeIndex).at().ptr();

(In reply to Paulo Matos from comment #32) > This patch unfortunately breaks the build on both arm and mips. > The problem is related with a non-existing function: > > ../../Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:338:93: error: > no matching function for call to > ‘JSC::CodeBlock::instructions(JSC::BytecodeIndex&)’ > const Instruction* instruction = > jit.baselineCodeBlock()->instructions(bytecodeIndex).at().ptr(); Oh, whoops, the bytecodeIndex needs to be the parameter to the at() not the instructions() call.

Reopened. This was reverted by r254632 (Bug 206301).

(In reply to Fujii Hironori from comment #34) > Reopened. This was reverted by r254632 (Bug 206301). I think this was relanded on https://bugs.webkit.org/show_bug.cgi?id=206361.

(In reply to Caio Lima from comment #35) Oops. Thanks.