[webkit-reviews] review denied: [Bug 25651] Allocation of BytecodeGenerator causes stack overflow : [Attachment 30251] update of patch for bug 25651

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Norbert Leser
<norbert.leser at nokia.com>'s request for review:
Bug 25651: Allocation of BytecodeGenerator causes stack overflow
https://bugs.webkit.org/show_bug.cgi?id=25651

Attachment 30251: update of patch for bug 25651
https://bugs.webkit.org/attachment.cgi?id=30251&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
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 at 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 at 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;
>  }