Bug 25651 - Allocation of BytecodeGenerator causes stack overflow
Summary: Allocation of BytecodeGenerator causes stack overflow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P2 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-08 13:31 PDT by Norbert Leser
Modified: 2009-06-24 00:27 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch for bug 25651 (4.49 KB, patch)
2009-05-08 13:32 PDT, Norbert Leser
no flags Details | Formatted Diff | Diff
List of comparative sunspider result URLs (4.32 KB, text/plain)
2009-05-08 13:34 PDT, Norbert Leser
ggaren: review-
Details
update of patch for bug 25651 (4.51 KB, patch)
2009-05-12 15:56 PDT, Norbert Leser
mjs: review-
Details | Formatted Diff | Diff
Updated patch for bug 24540 (4.35 KB, patch)
2009-06-16 15:02 PDT, Norbert Leser
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Norbert Leser 2009-05-08 13:31:40 PDT
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.
Comment 1 Norbert Leser 2009-05-08 13:32:58 PDT
Created attachment 30137 [details]
Proposed patch for bug 25651
Comment 2 Norbert Leser 2009-05-08 13:34:10 PDT
Created attachment 30138 [details]
List of comparative sunspider result URLs
Comment 3 Geoffrey Garen 2009-05-08 14:14:08 PDT
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.
Comment 4 Norbert Leser 2009-05-08 14:49:41 PDT
(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.
Comment 5 Norbert Leser 2009-05-12 15:56:27 PDT
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 6 Maciej Stachowiak 2009-05-21 20:10:51 PDT
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;
>  }
Comment 7 Norbert Leser 2009-06-16 15:02:11 PDT
Created attachment 31379 [details]
Updated patch for bug 24540

Updated patch addresses minor issue raised in Comment #6 From Maciej Stachowiak.
Comment 8 Oliver Hunt 2009-06-18 01:13:59 PDT
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 9 Maciej Stachowiak 2009-06-18 08:44:42 PDT
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.)
Comment 10 Eric Seidel (no email) 2009-06-24 00:27:03 PDT
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