The BytecodeGenerator objects are currently instantiated on stack (in Nodes.cpp), which takes up ~38kB per instance (each instance includes copy of JSC::CodeBlock with large SymbolTable, etc.). Specifically, since there is nested invocation (e.g., GlobalCode --> FunctionCode), the stack overflows immediately on Symbian hardware (max. 80 kB stack allowed). Proposed change allocates generator objects on heap. Performance impact (if any) should be negligible and change is proposed as general fix, rather than ifdef'd for SYMBIAN. I ran a couple of rounds of sunspider each, on code with stack and with heap allocation (see attached sunspider log). The results stray from slightly faster to slightly slower in each case and support the assertion of negligibility. Proposed patch is attached.
Created attachment 30137 [details] Proposed patch for bug 25651
Created attachment 30138 [details] List of comparative sunspider result URLs
Comment on attachment 30138 [details] List of comparative sunspider result URLs Seems reasonable. I'd prefer if you used OwnPtr instead of manual new/delete, though.
(In reply to comment #3) > (From update of attachment 30138 [details] [review]) > Seems reasonable. I'd prefer if you used OwnPtr instead of manual new/delete, > though. > I guess, that makes sense. I will verify that and supply an updates patch.
Created attachment 30251 [details] update of patch for bug 25651 Per suggestion from Geoffrey Garen, this update utilizes OwnPtr, instead of raw pointer for allocating heap memory.
Comment on attachment 30251 [details] update of patch for bug 25651 I don't think it's necessary to explicitly call .clear() on the OwnPtr, it will delete its contents on exit from the function anyway. Otherwise looks fine. r- to address this minor point. > Index: JavaScriptCore/ChangeLog > =================================================================== > --- JavaScriptCore/ChangeLog (revision 43518) > +++ JavaScriptCore/ChangeLog (working copy) > @@ -1,3 +1,22 @@ > +2009-05-11 Norbert Leser <norbert.leser@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + The BytecodeGenerator objects were instantiated on stack, which takes up ~38kB per instance > + (each instance includes copy of JSC::CodeBlock with large SymbolTable, etc.). > + Specifically, since there is nested invocation (e.g., GlobalCode --> FunctionCode), > + the stack overflows immediately on Symbian hardware (max. 80 kB). > + Proposed change allocates generator objects on heap. > + Performance impact (if any) should be negligible and change is proposed as general fix, > + rather than ifdef'd for SYMBIAN. > + > + * parser/Nodes.cpp: > + (JSC::ProgramNode::generateBytecode): > + (JSC::EvalNode::generateBytecode): > + (JSC::EvalNode::bytecodeForExceptionInfoReparse): > + (JSC::FunctionBodyNode::generateBytecode): > + (JSC::FunctionBodyNode::bytecodeForExceptionInfoReparse): > + > 2009-05-11 Sam Weinig <sam@webkit.org> > > Reviewed by Geoffrey "1" Garen. > Index: JavaScriptCore/parser/Nodes.cpp > =================================================================== > --- JavaScriptCore/parser/Nodes.cpp (revision 43514) > +++ JavaScriptCore/parser/Nodes.cpp (working copy) > @@ -1878,8 +1878,9 @@ void ProgramNode::generateBytecode(Scope > > m_code.set(new ProgramCodeBlock(this, GlobalCode, globalObject, source().provider())); > > - BytecodeGenerator generator(this, globalObject->debugger(), scopeChain, &globalObject->symbolTable(), m_code.get()); > - generator.generate(); > + OwnPtr<BytecodeGenerator> generator(new BytecodeGenerator(this, globalObject->debugger(), scopeChain, &globalObject->symbolTable(), m_code.get())); > + generator->generate(); > + generator.clear(); > > destroyData(); > } > @@ -1922,8 +1923,9 @@ void EvalNode::generateBytecode(ScopeCha > > m_code.set(new EvalCodeBlock(this, globalObject, source().provider(), scopeChain.localDepth())); > > - BytecodeGenerator generator(this, globalObject->debugger(), scopeChain, &m_code->symbolTable(), m_code.get()); > - generator.generate(); > + OwnPtr<BytecodeGenerator> generator(new BytecodeGenerator(this, globalObject->debugger(), scopeChain, &m_code->symbolTable(), m_code.get())); > + generator->generate(); > + generator.clear(); > > // Eval code needs to hang on to its declaration stacks to keep declaration info alive until Interpreter::execute time, > // so the entire ScopeNodeData cannot be destoyed. > @@ -1939,9 +1941,10 @@ EvalCodeBlock& EvalNode::bytecodeForExce > > m_code.set(new EvalCodeBlock(this, globalObject, source().provider(), scopeChain.localDepth())); > > - BytecodeGenerator generator(this, globalObject->debugger(), scopeChain, &m_code->symbolTable(), m_code.get()); > - generator.setRegeneratingForExceptionInfo(codeBlockBeingRegeneratedFrom); > - generator.generate(); > + OwnPtr<BytecodeGenerator> generator(new BytecodeGenerator(this, globalObject->debugger(), scopeChain, &m_code->symbolTable(), m_code.get())); > + generator->setRegeneratingForExceptionInfo(codeBlockBeingRegeneratedFrom); > + generator->generate(); > + generator.clear(); > > return *m_code; > } > @@ -2044,8 +2047,9 @@ void FunctionBodyNode::generateBytecode( > > m_code.set(new CodeBlock(this, FunctionCode, source().provider(), source().startOffset())); > > - BytecodeGenerator generator(this, globalObject->debugger(), scopeChain, &m_code->symbolTable(), m_code.get()); > - generator.generate(); > + OwnPtr<BytecodeGenerator> generator(new BytecodeGenerator(this, globalObject->debugger(), scopeChain, &m_code->symbolTable(), m_code.get())); > + generator->generate(); > + generator.clear(); > > destroyData(); > } > @@ -2071,9 +2075,10 @@ CodeBlock& FunctionBodyNode::bytecodeFor > > m_code.set(new CodeBlock(this, FunctionCode, source().provider(), source().startOffset())); > > - BytecodeGenerator generator(this, globalObject->debugger(), scopeChain, &m_code->symbolTable(), m_code.get()); > - generator.setRegeneratingForExceptionInfo(codeBlockBeingRegeneratedFrom); > - generator.generate(); > + OwnPtr<BytecodeGenerator> generator(new BytecodeGenerator(this, globalObject->debugger(), scopeChain, &m_code->symbolTable(), m_code.get())); > + generator->setRegeneratingForExceptionInfo(codeBlockBeingRegeneratedFrom); > + generator->generate(); > + generator.clear(); > > return *m_code; > }
Created attachment 31379 [details] Updated patch for bug 24540 Updated patch addresses minor issue raised in Comment #6 From Maciej Stachowiak.
Could you attach the output of sunspider-compare-results the logs you've attached make it difficult to verify the performance characteristics of the patch.
Comment on attachment 31379 [details] Updated patch for bug 24540 r=me (Oliver, I checked the SunSpider results by following the URLs and they indicate no regression.)
Committing to http://svn.webkit.org/repository/webkit/trunk ... M JavaScriptCore/ChangeLog M JavaScriptCore/parser/Nodes.cpp Committed r45066 http://trac.webkit.org/changeset/45066