RESOLVED FIXED 47107
Lazily create activation objects 
https://bugs.webkit.org/show_bug.cgi?id=47107
Summary Lazily create activation objects
Oliver Hunt
Reported 2010-10-04 13:23:21 PDT
Lazily create activation objects
Attachments
Patch (42.51 KB, patch)
2010-10-04 13:30 PDT, Oliver Hunt 		ggaren: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Oliver Hunt
Comment 1 2010-10-04 13:30:19 PDT
Created attachment 69674 [details] Patch
Geoffrey Garen
Comment 2 2010-10-04 13:53:33 PDT
Comment on attachment 69674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69674&action=review > JavaScriptCore/interpreter/Interpreter.cpp:3173 > - callFrame->r(dst) = JSValue(arguments); > - callFrame->r(unmodifiedArgumentsRegister(dst)) = JSValue(arguments); > + callFrame->r(argumentsRegister) = JSValue(arguments); > + callFrame->r(unmodifiedArgumentsRegister(argumentsRegister)) = JSValue(arguments); Since this turned out to be just a bug fix for a bug in a previous arguments optimization patch, I think you should land it separately with a test case.
Darin Adler
Comment 3 2010-10-04 13:57:47 PDT
Comment on attachment 69674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69674&action=review > JavaScriptCore/ChangeLog:13 > + This does make exception handling a little more dicey as The word “dicey” here makes me nervous. I wish you had said it differently. Is there enough test coverage? > JavaScriptCore/bytecode/CodeBlock.h:433 > + void createActivation(CallFrame*); > #if ENABLE(INTERPRETER) Why no blank line here? I would have had one. > JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1572 > +void BytecodeGenerator::createActivationIfNecessary() > +{ > + if (m_hasCreatedActivation) > + return; > + if (!m_codeBlock->needsFullScopeChain()) > + return; > + emitOpcode(op_create_activation); > + instructions().append(m_activationRegister->index()); > +} Should this function set m_hasCreatedActivation to true? If not, why not? It might need a comment. > JavaScriptCore/interpreter/Interpreter.cpp:131 > + if (checkTopLevel && skip--) { Same problem here that I spotted below. > JavaScriptCore/interpreter/Interpreter.cpp:213 > + if (checkTopLevel && skip--) { Same problem here that I spotted below. > JavaScriptCore/interpreter/Interpreter.cpp:2356 > + if (checkTopLevel && skip--) { Same problem here that I spotted below. > JavaScriptCore/jit/JITOpcodes.cpp:472 > + if (checkTopLevel && skip--) { Same problem here that I spotted below. > JavaScriptCore/jit/JITOpcodes.cpp:494 > + if (checkTopLevel && skip--) { Same problem here that I spotted below. > JavaScriptCore/jit/JITOpcodes.cpp:1590 > + if (checkTopLevel && skip--) { Same problem here that I spotted below. > JavaScriptCore/jit/JITOpcodes32_64.cpp:614 > + if (checkTopLevel && skip--) { Same problem here that I spotted below. > JavaScriptCore/jit/JITOpcodes32_64.cpp:643 > + if (checkTopLevel && skip--) { Same problem here that I spotted below. > JavaScriptCore/jit/JITStubs.cpp:2697 > + if (checkTopLevel && skip--) { If skip is 0 this will set it to -1. I don’t think you want that.
Oliver Hunt
Comment 4 2010-10-04 15:43:37 PDT
Committed r69045
Gavin Barraclough
Comment 5 2011-06-14 19:05:47 PDT
*** Bug 47718 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.