Bug 22385 – Lazily reparse FunctionBodyNodes on first execution.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Sam Weinig]

We can save a lot of memory if we don't keep the AST under FunctionBodyNodes around all the time and instead reparse for a tempory AST when need to generate bytecode.

[Sam Weinig]

Created attachment 25320 [details]
patch

[Darin Adler]

Comment on attachment 25320 [details]
patch

> +//Program_NoNode:
> +//    /* not in spec */
> +//    | SourceElements_NoNode
> +//;

Lets not check that in.

>  bool Lexer::isIdentStart(int c)
>  {
> -    return (category(c) & (Letter_Uppercase | Letter_Lowercase | Letter_Titlecase | Letter_Modifier | Letter_Other))
> -        || c == '$' || c == '_';
> +    return isASCIIAlpha(c) || c == '$' || c == '_' || (category(c) & (Letter_Uppercase | Letter_Lowercase | Letter_Titlecase | Letter_Modifier | Letter_Other));
>  }

A more efficient version of this would call "category" only if the character is non-ASCII. A patch I wrote a while back added isASCII to <wtf/ASCIICType.h> for just this sort of use.

I think you should land these smaller independent enhancements to Lexer separately.

>  static bool isDecimalDigit(int c)
>  {
> -    return (c >= '0' && c <= '9');
> +    return isASCIIDigit(c);
>  }

I don't see the point of making a function call for this every time. Could we just eliminate this function instead and use isASCIIDigit directly? Or make it inline?

>  bool Lexer::isHexDigit(int c)
>  {
> -    return (c >= '0' && c <= '9'
> -        || c >= 'a' && c <= 'f'
> -        || c >= 'A' && c <= 'F');
> +    return isASCIIHexDigit(c); 
>  }
>  
>  bool Lexer::isOctalDigit(int c)
>  {
> -    return (c >= '0' && c <= '7');
> +    return isASCIIOctalDigit(c);
>  }

Same comment.

> +    for (size_t i = 0; i < size; ++i)
> +        releaser.release(m_data->m_children[i]);
> +}
> +
> +
>  // ------------------------------ ProgramNode -----------------------------

Extra blank line here.

> +    // 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.
> +    children().shrinkCapacity(0);

Is this the right idiom? Didn't Maciej change clear() to do this?

> Index: parser/Nodes.h
> ===================================================================
> --- parser/Nodes.h	(revision 38582)
> +++ parser/Nodes.h	(working copy)
> @@ -170,6 +170,7 @@ namespace JSC {
>          virtual bool isResolveNode() const JSC_FAST_CALL { return false; }
>          virtual bool isBracketAccessorNode() const JSC_FAST_CALL { return false; }
>          virtual bool isDotAccessorNode() const JSC_FAST_CALL { return false; }
> +        virtual bool isFuncExprNode() const JSC_FAST_CALL { return false; } 
>  
>          virtual ExpressionNode* stripUnaryPlus() { return this; }
>  
> @@ -191,6 +192,7 @@ namespace JSC {
>  
>          virtual bool isEmptyStatement() const JSC_FAST_CALL { return false; }
>          virtual bool isReturnNode() const JSC_FAST_CALL { return false; }
> +        virtual bool isExprStatementNode() const JSC_FAST_CALL { return false; }

I don't think this needs "Node" in its name.

> +        void setData(ScopeNodeData* data) { m_data.set(data); }

This should take an auto_ptr argument, in my opinion. We probably also want to name it adoptData rather than setData.

> +static FunctionBodyNode* getFunctionBody(ProgramNode* program)

No need for "get" in the name of this function. You can just name the local variable "body" to avoid the conflict.

r=me

[Sam Weinig]

<rdar://problem/6390179>

[Sam Weinig]

Fixed in r38635.