Bug 66571

Created attachment 104538 [details] Proposed patch

Comment on attachment 104538 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=104538&action=review > Source/JavaScriptCore/runtime/JSObject.h:433 > inline JSObject* asObject(JSValue value) > { > + if (!value.isCell()) > + return 0; > return asObject(value.asCell()); This change is no good. No code should call asObject on a value if the value is not a cell (and therefore not an object). It's akin to a bad C++ cast. Is this change required to make your patch work?

> This change is no good. No code should call asObject on a value if the value is not a cell (and therefore not an object). It's akin to a bad C++ cast. > > Is this change required to make your patch work? Let me re-check....

Created attachment 104559 [details] updated patch I removed the extra code and one useless newline. You were right, the change was not required to make my patch work

Comment on attachment 104559 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=104559&action=review > Source/JavaScriptCore/interpreter/CallFrame.h:170 > + : globalData(global) > + , oldCallFrame(global->topCallFrame) Formatting mistake. These lines should be indented four more spaces. > Source/JavaScriptCore/interpreter/CallFrame.h:183 > + }; > } // namespace JSC Formatting mistake. There should be a blank line here.

Comment on attachment 104559 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=104559&action=review > Source/JavaScriptCore/interpreter/CallFrame.h:166 > + class TopCallFrame { I don’t think the class name here is right. It should be something like TopCallFrameSetter. Since this class is only used in three places in Interpreter.cpp I think it shouldn’t go into CallFrame.h. It can go into Interpreter.cpp. > Source/JavaScriptCore/interpreter/CallFrame.h:168 > + TopCallFrame(JSGlobalData* global, CallFrame* callFrame) I suggest taking a reference to global data instead of a pointer.

Created attachment 104592 [details] updated patch Thanks for the feedback Darin\!

Geoff: I tested my patch in both 64 and 32 bit, since I changed the patchOffsetGetByIdSlowCaseCall. Are there any other architectures that I should test it on?

Comment on attachment 104592 [details] updated patch getting weird results with performance tests... I have to re-check them

You'll need to update the offsets for ARM as well. Gavin can help you with this.

Comment on attachment 104592 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=104592&action=review Some of the setting and resetting of topCallFrame seems haphazard in the Interpreter. > Source/JavaScriptCore/interpreter/Interpreter.cpp:711 > CodeBlock* codeBlock = callFrame->codeBlock(); > bool isInterrupt = false; > + callFrame->globalData().topCallFrame = callFrame; Why does the first act of throwing an exception need to be to set topCallFrame? In theory, topCallFrame was already set when we entered the function that is now throwing an exception. On the other hand, I think you're missing an update of topCallFrame after you've thrown an exception and decided to return to a handler. You can put the update into unwindCallFrame(). > Source/JavaScriptCore/interpreter/Interpreter.cpp:934 > JSValue Interpreter::executeCall(CallFrame* callFrame, JSObject* function, CallType callType, const CallData& callData, JSValue thisValue, const ArgList& args) > { > + callFrame->globalData().topCallFrame = callFrame; Why here? Seems unnecessary. > Source/JavaScriptCore/interpreter/Interpreter.cpp:977 > JSValue Interpreter::executeCall(CallFra > } > > newCallFrame->init(newCodeBlock, 0, callDataScopeChain, callFrame->addHostCallFrameFlag(), argCount, function); > + TopCallFrameSetter topCallFrameSetter(callFrame->globalData(), newCallFrame); Here, you caught the CallTypeJS case, but missed the non-JS case. > Source/JavaScriptCore/interpreter/Interpreter.cpp:1030 > JSObject* Interpreter::executeConstruct(CallFrame* callFrame, JSObject* constructor, ConstructType constructType, const ConstructData& constructData, const ArgList& args) > { > + callFrame->globalData().topCallFrame = callFrame; Why here? Seems unnecessary. > Source/JavaScriptCore/interpreter/Interpreter.cpp:1055 > + TopCallFrameSetter topCallFrameSetter(callFrame->globalData(), newCallFrame); > + This is too soon, since newCallFrame is not yet fully initialized, and can change later in the function. A good rule of thumb is to update topCallFrame right after calling CallFrame::init, since that means you've just created a new CallFrame and fully initialized it. > Source/JavaScriptCore/interpreter/Interpreter.cpp:1217 > JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame, JSValue thisValue, ScopeChainNode* scopeChain) > { > + callFrame->globalData().topCallFrame = callFrame; Why here? Seems unnecessary. > Source/JavaScriptCore/interpreter/Interpreter.cpp:1227 > JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame, JSValue thisValue, int globalRegisterOffset, ScopeChainNode* scopeChain) > { > ASSERT(isValidThisObject(thisValue, callFrame)); > + callFrame->globalData().topCallFrame = callFrame; Why here? Seems unnecessary. > Source/JavaScriptCore/interpreter/Interpreter.cpp:1658 > #endif > > + *topCallFrameSlot = callFrame; > #if ENABLE(COMPUTED_GOTO_INTERPRETER) Why here? Seems unnecessary. > Source/JavaScriptCore/interpreter/Interpreter.cpp:4188 > goto vm_throw; > functionReturnValue = result; > > + *topCallFrameSlot = callFrame; Why here? callEval() should restore topCallFrame for you. > Source/JavaScriptCore/interpreter/Interpreter.cpp:4257 > + *topCallFrameSlot = newCallFrame; > newCallFrame->init(0, vPC + OPCODE_LENGTH(op_call), scopeChain, callFrame, argCount, asObject(v)); Slightly better to switch these two lines of code, so topCallFrame doesn't point at newCallFrame until newCallFrame is in a valid state. > Source/JavaScriptCore/interpreter/Interpreter.cpp:4427 > + *topCallFrameSlot = newCallFrame; > newCallFrame->init(0, vPC + OPCODE_LENGTH(op_call_varargs), scopeChain, callFrame, argCount, asObject(v)); Same comment here. > Source/JavaScriptCore/interpreter/Interpreter.cpp:4438 > + *topCallFrameSlot = callFrame; This needs to happen before CHECK_FOR_EXCEPTION(), to ensure that you put things back into a valid state before jumping somewhere else. > Source/JavaScriptCore/interpreter/Interpreter.cpp:4518 > + *topCallFrameSlot = callFrame; > if (callFrame->hasHostCallFrameFlag()) > return returnValue; Slightly better to change around the order of these two statements, since you don't need to update topCallFrame if you're returning back to C++. (The C++ code should update for you.) > Source/JavaScriptCore/interpreter/Interpreter.cpp:4761 > CHECK_FOR_EXCEPTION(); > functionReturnValue = returnValue; > > + *topCallFrameSlot = callFrame; Update needs to happen before CHECK_FOR_EXCEPTION. You missed the initial setting of topCallFrame in the ConstructTypeHost case here. > Source/JavaScriptCore/interpreter/Interpreter.cpp:4940 > + *topCallFrameSlot = callFrame; Why here? Seems unnecessary, if exception unwinding does its job correctly. > Source/JavaScriptCore/interpreter/Interpreter.cpp:4964 > codeBlock = callFrame->codeBlock(); > + *topCallFrameSlot = callFrame; > vPC = codeBlock->instructions().begin() + handler->target; If you fix throwException to update topCallFrame correctly, this line shouldn't be necessary. > Source/JavaScriptCore/interpreter/Interpreter.cpp:5124 > + *topCallFrameSlot = callFrame; > vPC = codeBlock->instructions().begin() + handler->target; Same comment here.

> > Source/JavaScriptCore/interpreter/Interpreter.cpp:711 > > CodeBlock* codeBlock = callFrame->codeBlock(); > > bool isInterrupt = false; > > + callFrame->globalData().topCallFrame = callFrame; > > Why does the first act of throwing an exception need to be to set topCallFrame? in throwException(), addErrorInfo() needs the updated callFrame to work properly. I update the topCallFrame before the first if statement to set it in case it branches. > On the other hand, I think you're missing an update of topCallFrame after you've thrown an exception and decided to return to a handler. You can put the update into unwindCallFrame(). If I only update the callFrame inside unwindCallFrame, my code would fail jsc-tests. As I mentioned before, addErrorInfo() needs the updated callFrame to work, and addErrorInfo() is called before unwindCallFrame() Should I keep it like this or what would be the right course of action?

(In reply to comment #12) > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:711 > > > CodeBlock* codeBlock = callFrame->codeBlock(); > > > bool isInterrupt = false; > > > + callFrame->globalData().topCallFrame = callFrame; > > > > Why does the first act of throwing an exception need to be to set topCallFrame? > > in throwException(), addErrorInfo() needs the updated callFrame to work properly. I update the topCallFrame before the first if statement to set it in case it branches. If you saw cases where addErrorInfo() saw the wrong callFrame, perhaps they were caused by some of the cases of missing updates I listed. If you do the job right, addErrorInfo, like all C++ code, should always have a coherent view of topCallFrame. > > On the other hand, I think you're missing an update of topCallFrame after you've thrown an exception and decided to return to a handler. You can put the update into unwindCallFrame(). > > If I only update the callFrame inside unwindCallFrame, my code would fail jsc-tests. As I mentioned before, addErrorInfo() needs the updated callFrame to work, and addErrorInfo() is called before unwindCallFrame() The update at the top of Interpreter::throwException will not make topCallFrame correct after you've thrown an exception and decided to return to a handler. Consider this case: (function () { try { (function() { throw "error"; })(); } catch(exception) { // topCallFrame is now wrong. } })(); topCallFrame needs to point to the callFrame of the exception handler, which can be zero or more frames up the stack from the frame that throws the exception.

Created attachment 104894 [details] updated arm-ready patch I added the changes suggested by Geoff. I decided to update both in unwindCallFrame() and when an exception is thrown. I also added some asserts where I removed updates to topCallFrame just to make sure that the callframes at those points were correct. This patch has also been modified to work with arm processors. Hopefully I didn't miss anything

Created attachment 105022 [details] Complete results of performance tests made on this patch

Created attachment 105023 [details] sunspider test results

Created attachment 105024 [details] v8 results (outside web browser)

Created attachment 105025 [details] v8 results (in web browser)

Created attachment 105026 [details] kraken results

Created attachment 105028 [details] new and improved patch Removed some assert statements. I included some perfromance test results that I ran yesterday/today. It looks like no measurable speed regression was introduced by this patch.

Created attachment 105076 [details] patch added one more TopCallFrameSetter in execute(CallFrameClosure) to assure that we are updating and resetting topCallFrame before and after we execute arbitrary js code that can change our topCallFrame

performance figures are looking very similar to the ones I posted. I'll keep running them and upload the new ones.

Created attachment 105085 [details] new perf test results

Comment on attachment 105076 [details] patch r=me

Comment on attachment 105076 [details] patch Clearing flags on attachment: 105076 Committed r93755: <http://trac.webkit.org/changeset/93755>

All reviewed patches have been landed. Closing bug.