Summary: | Allocation of BytecodeGenerator causes stack overflow | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Norbert Leser <norbert.leser> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Critical | CC: | hausmann, laszlo.gombos | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | S60 Hardware | ||||||||||||
OS: | S60 3rd edition | ||||||||||||
Attachments: |
|
Description
Norbert Leser
2009-05-08 13:31:40 PDT
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 |