Bug 74509 - Interpose CodeNode between ScopeNode and ProgramNode et al
Summary: Interpose CodeNode between ScopeNode and ProgramNode et al
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Wingo
URL:
Keywords:
Depends on: 79112
Blocks: 74725
  Show dependency treegraph
 
Reported: 2011-12-14 08:50 PST by Andy Wingo
Modified: 2013-10-31 11:36 PDT (History)
3 users (show)

See Also:


Attachments
Patch (20.37 KB, patch)
2011-12-14 09:06 PST, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch (20.33 KB, patch)
2012-01-18 10:28 PST, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch (16.33 KB, patch)
2012-02-28 08:45 PST, Andy Wingo
no flags Details | Formatted Diff | Diff
fix a loose ends from rebasing (16.30 KB, patch)
2012-03-07 10:53 PST, Andy Wingo
fpizlo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Wingo 2011-12-14 08:50:45 PST
Currently in JS, variables are bound at functions, and in eval code, and in toplevel program code.  Harmony / ES6 introduces a new way to bind variables, in block scopes.  See bug 31813 for more.

The patch to be attached separates the concern of function binding with the other concerns of functions, eval code, and eval code (namely, holding a codeblock).  It does so by introducing a new class, CodeNode, between ScopeNode and ProgramNode.

A future patch will add BlockScopeNode whose superclass is ScopeNode.
Comment 1 Andy Wingo 2011-12-14 08:51:54 PST
Er, "separates the concern of variable binding".
Comment 2 Andy Wingo 2011-12-14 08:52:29 PST
Also, "eval code, and program code".  Sorry again for the noise.
Comment 3 Andy Wingo 2011-12-14 09:06:57 PST
Created attachment 119230 [details]
Patch
Comment 4 Gavin Barraclough 2012-01-17 10:46:28 PST
I think this patch may be going in the wrong direction.  If the purpose of this patch is to allow BlockScopeNodes that subclass ScopeNode, and the effect of this patch is to move the needsActivation & capture state out into a subclass, it appears that you are predicating this change on early tear-off onto a scope node? – but I think we may really want lazy tear-off using an activation.

You really need to address my comments in https://bugs.webkit.org/show_bug.cgi?id=74633 to further elucidate on your design before proceeding.
Comment 5 Andy Wingo 2012-01-17 12:47:10 PST
Thanks for the note, Gavin.  I have been thinking about that comment for a little while now, and will address it tomorrow.  Briefly now though, I'm not certain that this change (and a subsequent BlockScopeNode) necessitates "early tear-off".  This patch would seem to work equally well for both strategies.  But, more tomorrow.

Happy hacking :)
Comment 6 Andy Wingo 2012-01-18 10:28:20 PST
Created attachment 122957 [details]
Patch
Comment 7 Andy Wingo 2012-02-28 08:39:28 PST
Attaching a rebase on top of bug 79112
Comment 8 Andy Wingo 2012-02-28 08:45:10 PST
Created attachment 129266 [details]
Patch
Comment 9 Andy Wingo 2012-03-07 10:33:08 PST
Performance-neutral on master: 5.997s in my parse test[0] vs 5.983s for trunk.

[0] bug 79776 comment 12

I have some pending patches for lazy tear-off that haven't quite congealed yet.  This is a step in that direction, but if it's not clear to you, dear reviewer, then it's OK to hold off on reviewing this one.
Comment 10 Andy Wingo 2012-03-07 10:53:16 PST
Created attachment 130648 [details]
fix a loose ends from rebasing
Comment 11 Filip Pizlo 2013-10-31 11:36:15 PDT
Comment on attachment 130648 [details]
fix a loose ends from rebasing

This probably needs a massive rebase.