Bug 16585 - Optimize variable declarations
Summary: Optimize variable declarations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.4
: P3 Normal
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-12-23 07:51 PST by Maciej Stachowiak
Modified: 2007-12-24 15:45 PST (History)
0 users

See Also:


Attachments
optimize var statements - 3.7% speedup (26.43 KB, patch)
2007-12-23 07:55 PST, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
cleaned up version (now measures as 3.5% speedup) (40.17 KB, patch)
2007-12-24 01:14 PST, Maciej Stachowiak
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2007-12-23 07:51:49 PST
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.
Comment 1 Maciej Stachowiak 2007-12-23 07:55:24 PST
Created attachment 18071 [details]
optimize var statements - 3.7% speedup
Comment 2 Eric Seidel (no email) 2007-12-23 15:58:46 PST
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.
Comment 3 Maciej Stachowiak 2007-12-24 01:14:05 PST
Created attachment 18090 [details]
cleaned up version (now measures as 3.5% speedup)
Comment 4 Eric Seidel (no email) 2007-12-24 01:44:33 PST
Comment on attachment 18090 [details]
cleaned up version (now measures as 3.5% speedup)

Looks good to me.