Bug 16585 – Optimize variable declarations

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Maciej Stachowiak]

Variable declaration statements can be replaced at parse time with assignments if there are initializers, or nothing at all if there aren't. The actual declaration logic is handled on entering the function body anyway.

[Maciej Stachowiak]

Created attachment 18071 [details]
optimize var statements - 3.7% speedup

[Eric Seidel (no email)]

Comment on attachment 18071 [details]
optimize var statements - 3.7% speedup

So my comments deal entirely with code readability.  AFAICT, the functionality looks fine:

ExprStatementNode<false> and ExprStatementNode<true> should have nicely named typdefs.  Probably ExprStatementNode should also change names to some sort of ExprStatementNodeBase or the like.

As part of that <isVar> needs a more descriptive name.  Small functions would be better done using the typdef classes, larger functions can be defined using ExprStatementNode<betterNameThanIsVar>::

If you look at the formatted diff, it appears you added a whole bunch of extra whitespace.

I think passing int attrs as part of appendToVarDeclarationList and using DeclarationStacks::IsConstant and DeclarationStacks::HasInitializerat the callsites would be clearer.  At least at the callsites. :)

 1807       ForNode(ExpressionNode* e1, ExpressionNode* e2, ExpressionNode* e3, StatementNode* s, bool e1WasVarDecl) KJS_FAST_CALL

I was trying to come up with a cleaner way of expressing the same.  Using an enum "FirstExprIsVarDecl" instead of a bool would lead to cleaner callsites... I guess that's about the best I can come up with for right now.

If you fix at least some of those readability issues, this is totally fine for landing.  Congrats on such an awesome speedup.

[Maciej Stachowiak]

Created attachment 18090 [details]
cleaned up version (now measures as 3.5% speedup)

[Eric Seidel (no email)]

Comment on attachment 18090 [details]
cleaned up version (now measures as 3.5% speedup)

Looks good to me.