Bug 31813

Summary: [ES6] Add support for block scope const
Product: WebKit Reporter: Erik Arvidsson <arv>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: antonm, barraclough, buildbot, commit-queue, damian.chojna, fpizlo, ggaren, joepeck, jwalden+bwo, keith_miller, mark.lam, m.goleb+bugzilla, mike, mjs, mmirman, msaboff, ojan, oliver, rniwa, saam, syoichi, wingo, xan.lopez, ysuzuki, zwarich
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://wiki.ecmascript.org/doku.php?id=harmony:const
Bug Depends on: 142944, 147063    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch
none
patch
none
WIP
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
patch
fpizlo: review+, buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
patch none

Description Erik Arvidsson 2009-11-23 14:09:56 PST
JSC currently supports const but the current implementation use function scope. For ES Harmony, const should have block scoping.

var x = 1
{
  const x = 2;
  print(x); // 2
}
print(x); // 1
x = 3;
print(x); // 3
Comment 1 Andy Wingo 2011-11-16 06:48:26 PST
I would like to implement this, so if you don't mind I will use this bug for stating the requirements, coming to a strategy, and fleshing out an implementation.

To summarize, there are currently two ES modes:

* Strict
* "Classic" (non-strict)

ES.next adds a third:

* "Harmony"

In classic mode, `const' has no specification.  In JSC it is like `var', and hoisted to function scope.

In strict mode, `const' is a syntax error.

In harmony mode, `const' is block-scoped, with a so-called "temporal dead zone": when the var is first made, it is logically populated with some sentinel value.  If the var is accessed before it is set, that is a runtime error.  If it is set twice, that is a runtime error.  In some cases the compiler can elide the temporal dead zone, but not in general.

There are also some wrinkles that in top-level strict contexts, `const' should create non-writable, non-configurable properties.  Full details on the linked page.
Comment 2 Andy Wingo 2011-11-16 06:56:28 PST
So that's the spec, as it appears to be shaping up.  As far as implementation goes, SpiderMonkey and V8 appear to be choosing different strategies:

  V8: https://mail.mozilla.org/pipermail/es-discuss/2011-November/018480.html
  SM: https://mail.mozilla.org/pipermail/es-discuss/2011-November/018462.html

V8's approach is more conservative, preferring to add a mode rather than to add `const' to strict mode.

SM is more optimistic (for better or for worse); they seem to think they can migrate their users to block-scoped `const' and everything will be fine.

JSC currently forbids `const' in strict mode, and has some crufty function-scope support for `const' in classic mode.

Now here's the thing, JSC currently has no harmony "mode": there's just strict and non-strict.  We have to make a choice as to whether to continue like this and support some subset of harmony features within strict mode, or to add another mode, or to decide not to implement harmony, such as it is.

My motivation here is that I have a bunch of SM code that uses let, const, and destructuring, and I would like to convert it to use JSC.  I would appreciate feedback from JSC folk as to whether this is a good idea, or whether I should use another engine.  If you are amenable to block scoping somehow, I am happy to implement it.

Andy
Comment 3 Andy Wingo 2011-11-16 07:24:53 PST
As one data point, I think my initial strategy is going to be to not add another mode.  It's probably OK to add `let' and `const' to strict mode, as SpiderMonkey does.  I'll add this implementation under a compile-time flag.
Comment 4 Erik Arvidsson 2011-11-16 10:53:59 PST
(In reply to comment #1)
> In harmony mode, `const' is block-scoped, with a so-called "temporal dead zone": when the var is first made, it is logically populated with some sentinel value.  If the var is accessed before it is set, that is a runtime error.  If it is set twice, that is a runtime error.  In some cases the compiler can elide the temporal dead zone, but not in general.

Please read the latest draft before you start.

http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts

There is no sentinel and const declaration requires an initializer.

ConstDeclaration :
  const ConstBindingList ;

ConstBindingList :
  ConstBinding
  ConstBinding List , ConstBinding

ConstBinding :
  BindingIdentifier Initialiser
  BindingPattern Initialiser

 The draft spec does not yet cover const in for, for-in and for-of loops.

The URL of this bug points to the wiki page which clearly states that there is no temporal dead zone.

> There are also some wrinkles that in top-level strict contexts, `const' should create non-writable, non-configurable properties.  Full details on the linked page.

Also, in Harmony there is no global object like that. Global variables does not create properties on global.
Comment 5 Gavin Barraclough 2011-11-16 12:51:19 PST
I'm not sure that we want to bleed partial Harmony support into JSC - so I think we should probably keep this out of strict es 5.1 mode.  That said, if we initially land this behind an ifdef, we don't necessarily need to make a decision on this from the outset, the guards will presumably still be there on the other side of the ifdef.

I guess there are really three options here - always allow block const, only allow based on state external to the script, (<script> tag or command line parameter), or a "use strict" style pragma in-band.  Whilst it doesn't currently sound like the ES committee are going to go for another "use strict" like directive, this might be useful for internal development purposes (e.g. "use webkit-harmony").

I doubt anyone relies on the precise behavior of our crufty function-scoped const, so it may end up making sense to replace the classic const implementation with a block-scoped one, rather than supporting multiple incompatible behaviors.
Comment 6 Andy Wingo 2011-11-16 13:20:50 PST
(In reply to comment #4)
> Please read the latest draft before you start.
> 
> http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts

Thanks for the pointer.  I still don't understand all of the tc39 process.

> There is no sentinel and const declaration requires an initializer.

Though the const declaration requires an initializer, there is still a sentinel AFAIU.  See for example the V8 mail I linked to before, or Brendan's reply to that.

> > There are also some wrinkles that in top-level strict contexts, `const' should create non-writable, non-configurable properties.  Full details on the linked page.
> 
> Also, in Harmony there is no global object like that. Global variables does not create properties on global.

Well, I'll take a look again.  I was basing my interpretations on es-discuss proceedings from the last month or so.

Thanks for the valuable feedback, though it does make me a little less sure of what the spec is right now ;)
Comment 7 Erik Arvidsson 2011-11-16 15:02:05 PST
(In reply to comment #6)
> Well, I'll take a look again.  I was basing my interpretations on es-discuss proceedings from the last month or so.

I stand corrected. There is still a temporal dead zone requirement for const in harmony. The reason is that you can have forward references.
Comment 8 Andy Wingo 2011-11-17 04:06:00 PST
Harmony (or ES.next or ES6, which are the same thing) is shaping up to
be a pretty big language.  There is consensus on some pieces, and some
pieces are more experimental.  From an implementor's perspective it is a
waste of time to implement something that will change significantly in
the future, so we need to identify a stable subset of Harmony to
consider implementing.

Given that we are looking at block-scoped const in the broader Harmony
context, I thought it would be useful to look at what V8 has identified
to be a stable subset, and what they are doing.

V8 implements (1) harmony proxies, (2) collections (maps, sets, and weak
maps), (3) typeof, and (4) block scoping (const and let).

In V8, all harmony support is currently off by default.  There are
separate runtime flags to enable each one of these four areas.  There is
a master --harmony switch to enable all of them.  Even when it is off,
Harmony support is always compiled into V8, without an #ifdef.

1. Proxies are implemented as a separate JSReceiver implementation,
JSObject being the other one.  There are about 100 mentions of JSProxy
in the V8 source -- so a fair amount of code, but not enormous.  Proxies
do have a couple of runtime helper stubs, but otherwise don't affect
code generation.  That's all I have to say on proxies.

2. Harmony collections (sets, maps, and weak maps) likewise don't affect
the core implementation too much, as they are essentially libraries.

3. In Harmony, typeof(null) is 'null', not 'object'.  This affects code
generation in some places, mostly positively.  I don't know how much it
would break the web if it were on by default, though.

4. OK, so we are finally down to scoping.  This is the most complicated
piece.  I have described the specification for block-scoped const in bug
31813 comment 2.  V8 implements it entirely, except for `const' at
toplevel.  `let' is very similar to `const', except that the bindings
are mutable.  There is still a "temporal dead zone".

I should mention first that like JSC, V8 has a legacy `const' mode.  It
does not reserve the `let' token in "classic" mode (see
comment 1).  In strict mode, it raises early errors for `const' and
`let'.  In harmony mode, it implements harmony scoping.

Crankshaft does not handle `let' or harmony `const' yet, so using these
constructs is currently a speed penalty relative to `var'.  Of course
the read barrier ("temporal dead zone") is also a performance lose, but
probably not too significant.  V8 does omit the read barrier in one
case; from full-codegen-ia32.cc:

 // The check
 // can be skipped in the following situation: we have a LET or CONST
 // binding in harmony mode, both the Variable and the VariableProxy have
 // the same declaration scope (i.e. they are both in global code, in the
 // same function or in the same eval code) and the VariableProxy is in
 // the source physically located after the initializer of the variable.
 //
 // We cannot skip any initialization checks for CONST in non-harmony
 // mode because const variables may be declared but never initialized:
 //   if (false) { const x; }; var y = x;
 //
 // The condition on the declaration scopes is a conservative check for
 // nested functions that access a binding and are called before the
 // binding is initialized:
 //   function() { f(); let x = 1; function f() { x = 2; } }
 //

Note that if you come from a functional programming backgroud, harmony
`let' is more like `letrec' (or `letrec*') from other languages, hence
the complications.  The general strategy from "Fixing Letrec: A Faithful
Yet Efficient Implementation of Scheme's Recursive Binding Construct",
by Oscar Waddell, Dipanwita Sarkar, and R. Kent Dybvig could apply here.

Anyway, sentinel values and read and (in the case of `const') write
barriers.  There's more though.

In harmony mode, every call to `eval' is a strict-mode eval call.  This
behavior is enabled by the harmony-scoping flag.

In harmony mode, you need to check for multiple declarations of the same
let-bound or const-bound variable.  You need to check for incompatible
bindings: `var' and `let' declarations in the same scope, or `var'
hoisted past a `let'.

In harmony mode, a SourceElement may be not only a Statement or a
FunctionDeclaration, but also a LetDeclaration or a ConstDeclaration.
ConstDeclarations have non-optional initializers.

Function declarations are scoped using `let' in harmony mode, and thus
are block-scoped, not hoisted.  For named function expressions, the
closure binding between the name and the function is made using `const',
in harmony mode.

In harmony mode, a Block is parsed as a sequence of SourceElements, not
Statements.

Catch vars are bound using `let' in harmony mode.

That's it, I think.
Comment 9 Andy Wingo 2011-11-17 08:19:17 PST
To continue a little more here, in V8 there is no support for enabling harmony mode within a script or a function.  It's an outside flag that is on or off.  This is easier than the strict mode case, in which the strict mode flag has to be plumbed around everywhere.

It also appears to be orthogonal to strict mode, though that is probably a bug: http://code.google.com/p/v8/issues/detail?id=1829
Comment 10 Andy Wingo 2011-11-17 08:52:07 PST
Thanks for the notes, Gavin.

(In reply to comment #5)
> I guess there are really three options here - always allow block const, only allow based on state external to the script, (<script> tag or command line parameter), or a "use strict" style pragma in-band.  Whilst it doesn't currently sound like the ES committee are going to go for another "use strict" like directive, this might be useful for internal development purposes (e.g. "use webkit-harmony").

I'm open to any of these.  FWIW as I noted V8 went with the "external state" solution.  I don't know what the right answer is.

> I doubt anyone relies on the precise behavior of our crufty function-scoped const, so it may end up making sense to replace the classic const implementation with a block-scoped one, rather than supporting multiple incompatible behaviors.

Again, just for what it's worth, V8 preserved the old behavior and added a new const_harmony mode.  I really don't have a good idea about what enabling block const would do in a broader web context.
Comment 11 Andy Wingo 2011-11-21 02:47:02 PST
The plot thickens on the V8 side.  Chromium hacker Steven Keuchel notes in http://code.google.com/p/v8/issues/detail?id=1829#c3 that they would like to enable some harmony features within strict mode by default, if I understand him correctly.

I think I will do what they are planning to do, and just have one global flag to turn harmony on or off at runtime, and have that flag behind an #ifdef in the beginning.  Then we will only recognize `let' and `const' within strict-mode code, when the harmony flag is on.  We can talk about the default #ifdef and runtime flag settings later on.
Comment 12 Andy Wingo 2011-11-21 10:21:58 PST
Related: bug 72889 for runtime options.
Comment 13 Andy Wingo 2011-11-24 07:57:10 PST
FYI, V8 now has three modes: classic, strict, and "extended":

  http://code.google.com/p/v8/source/detail?r=10062
Comment 14 Gavin Barraclough 2011-11-24 23:06:55 PST
(In reply to comment #13)
> FYI, V8 now has three modes: classic, strict, and "extended":
> 
>   http://code.google.com/p/v8/source/detail?r=10062

Given the spec defined mechanism by which strict mode is enabled (an inline directive in the source file), it's a little redundant to make this a command-line option, and would take a little extra plumbing into the parser to make this work (though not a lot to be fair, we'll probably need to tag SourceCode/SourceProvider objects with a language mode anyway if we want to support ES6).
Comment 15 Andy Wingo 2011-11-25 08:17:45 PST
(In reply to comment #14)
> (In reply to comment #13)
> > FYI, V8 now has three modes: classic, strict, and "extended":
> 
> Given the spec defined mechanism by which strict mode is enabled (an inline directive in the source file), it's a little redundant to make this a command-line option

Agreed.  I think the command-line / run-time option is just to be conservative, and not enable harmony by default (yet?).

A flag of some sort is still needed to control visibility of the new libraries in ES6: Map, Set, etc.  In ES6 mode you would have them in the global environment, AFAIU, but not in "extended" mode.
Comment 16 Gavin Barraclough 2011-11-25 11:24:43 PST
> A flag of some sort is still needed to control visibility of the new libraries in ES6: Map, Set, etc.  In ES6 mode you would have them in the global environment, AFAIU, but not in "extended" mode.

Actually that might not be an issue.  The plan for ES6 is that the global object won't be in scope and instead there will be a module system (it will be imported modules that are available in the outer scope in ES6 code, not the properties of the global object).  As such, ES6 will likely not define that these properties exist on the global object – rather that the containing modules must exist.
Comment 17 Andy Wingo 2011-11-30 03:35:32 PST
Good points regarding modules.

I'm going to work on this a bit more for the rest of the week and weekend (the GTK+ port has a hackfest).  The tentative plan is:

 1. New code is guarded by ENABLE(HARMONY)

 2. There is also Options::harmony, and sub-option Options::harmonyBlockScoping, to allow runtime control, at least until things settle down

 3. Adding an "extended" mode does not seem necessary yet, as the additions are not incompatible with old strict-mode code.  In short, I plan to enable block-scoped `let' and `const' in strict-mode code when the global Options::harmonyBlockScoping flag is on.

 4. We will have to add new binding modes, CONST_HARMONY and LET.

The actual implementation mechanism is likely to be a generalization of what's used by `catch' blocks, but I don't understand everything there yet.

Comments welcome.
Comment 18 Andy Wingo 2011-12-04 07:57:07 PST
So, a status update.  I added the compile-time and run-time flags for harmony block scoping.  When the flags are on, we parse `const' in strict-mode as having block scope.  I check correctly for conflicting variable declarations, I think.

I extended the Parser::Scope object to track block-scoped declarations.  I push a scope when parsing block statements in harmony mode, and if the scope contains lexical bindings, I make a BlockScopeNode instad of a BlockStatementNode.  This works but does not currently catch

if (1)
  const x = 10;

which should be an error AFAIK.  Block-scoped declarations outside of functions are also collected at the top level; currently they result in lexical bindings, not read-only, non-configurable properties, but that can be fixed.

Anyway, that's the parser and the AST.  It used to be that ProgramNode, EvalNode, and FunctionBodyNode all descended from ScopeNode, but I changed them to descend from an interposed CodeNode, and then BlockScopeNode descends from ScopeNode but without the toplevel baggage of the others.  I'm not sure if this was useful though.

Implementation-wise, things are pretty terrible: I have a JSBlockScopeObject like the StaticScopeObject, but with more slots.  Entering in a block scope will allocate that object and push it on the scope chain.  Currently findScopedProperty punts in that case; something to fix.  So the lookup is not fast.  It's a shame though; in strict mode, it should be possible to do much better (with assignment conversion).

There is a new VM op to create a blockscope.  Perhaps that should merge with JSStaticScope.

I have not yet modified either JIT.  I have not yet implemented the read- or write-barriers.  They will probably require additional vm ops.

Unless you want the patches now, I'll clean them up before posting for review.
Comment 19 Andy Wingo 2011-12-16 09:50:45 PST
I have a prototype up at bug 74725.  It depends on a few more patches in bugzilla; you can follow the dependencies to see.  I still need to implement the temporal dead zone, figure out a better versioning strategy, and write a test suite, but much of that will probably have to wait until after the holiday season.
Comment 20 Gavin Barraclough 2012-03-07 00:31:30 PST
*** Bug 14611 has been marked as a duplicate of this bug. ***
Comment 21 Gavin Barraclough 2012-03-07 00:32:49 PST
*** Bug 28236 has been marked as a duplicate of this bug. ***
Comment 22 Gavin Barraclough 2012-03-07 00:33:04 PST
*** Bug 33849 has been marked as a duplicate of this bug. ***
Comment 23 Saam Barati 2015-07-09 17:54:35 PDT
Created attachment 256546 [details]
patch

This currently also has: https://bugs.webkit.org/show_bug.cgi?id=142944
baked into it. This will obviously change once 142944 lands.

Still need to do:
- remove old tests
- add new tests
Comment 24 Saam Barati 2015-07-09 18:00:36 PDT
*** Bug 143654 has been marked as a duplicate of this bug. ***
Comment 25 Saam Barati 2015-07-10 13:39:54 PDT
Created attachment 256608 [details]
patch

This is ready to go after "let" lands.
All that is left for me to do is remove old, incorrect, tests and update old tests results.
New tests are written.
Comment 26 Saam Barati 2015-07-10 13:51:55 PDT
Created attachment 256611 [details]
patch

same as before but removed old code and added one new test.
Comment 27 Saam Barati 2015-07-13 23:45:02 PDT
Created attachment 256761 [details]
WIP
Comment 28 Saam Barati 2015-07-16 14:18:29 PDT
Created attachment 256926 [details]
patch
Comment 29 Build Bot 2015-07-16 15:09:32 PDT
Comment on attachment 256926 [details]
patch

Attachment 256926 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5703103198265344

Number of test failures exceeded the failure limit.
Comment 30 Build Bot 2015-07-16 15:09:37 PDT
Created attachment 256930 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 31 Build Bot 2015-07-16 15:21:59 PDT
Comment on attachment 256926 [details]
patch

Attachment 256926 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5844178378424320

Number of test failures exceeded the failure limit.
Comment 32 Build Bot 2015-07-16 15:22:02 PDT
Created attachment 256934 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 33 Saam Barati 2015-07-16 16:33:49 PDT
Looks like a lot of tests use "const" as if it's a global property.
I'll fix tests to switch to "var".
Comment 34 Saam Barati 2015-07-16 18:59:44 PDT
Created attachment 256945 [details]
patch
Comment 35 Build Bot 2015-07-16 19:51:43 PDT
Comment on attachment 256945 [details]
patch

Attachment 256945 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5409610903060480

New failing tests:
inspector/console/console-table.html
Comment 36 Build Bot 2015-07-16 19:51:49 PDT
Created attachment 256951 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 37 Build Bot 2015-07-16 19:56:25 PDT
Comment on attachment 256945 [details]
patch

Attachment 256945 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6177052839378944

New failing tests:
inspector/console/console-table.html
Comment 38 Build Bot 2015-07-16 19:56:30 PDT
Created attachment 256953 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 39 Filip Pizlo 2015-07-16 19:59:17 PDT
Comment on attachment 256945 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256945&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1447
> -Variable BytecodeGenerator::variable(const Identifier& property)
> +std::pair<Variable, BytecodeGenerator::ScopeType> BytecodeGenerator::resolveVariable(const Identifier& property)

The idea of the Variable class was to hold all of the things that this method returned.  Why can't ScopeType just be another field of Variable?
Comment 40 Saam Barati 2015-07-17 11:25:09 PDT
Comment on attachment 256945 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256945&action=review

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1447
>> +std::pair<Variable, BytecodeGenerator::ScopeType> BytecodeGenerator::resolveVariable(const Identifier& property)
> 
> The idea of the Variable class was to hold all of the things that this method returned.  Why can't ScopeType just be another field of Variable?

There is no reason why it shouldn't. Variable's now have a notion of isLexicallyScoped().
Also, they have a notion of isConst(), where 
isConst() := isLexicallyScoped() && isReadOnly()
Comment 41 Saam Barati 2015-07-18 18:25:37 PDT
Created attachment 257039 [details]
patch

I'm seeing how EWS likes this as I update my machine.
Comment 42 Build Bot 2015-07-18 19:16:16 PDT
Comment on attachment 257039 [details]
patch

Attachment 257039 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4780413226582016

Number of test failures exceeded the failure limit.
Comment 43 Build Bot 2015-07-18 19:16:20 PDT
Created attachment 257040 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 44 Build Bot 2015-07-18 19:21:34 PDT
Comment on attachment 257039 [details]
patch

Attachment 257039 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5796731337834496

Number of test failures exceeded the failure limit.
Comment 45 Build Bot 2015-07-18 19:21:39 PDT
Created attachment 257041 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 46 Saam Barati 2015-07-18 21:49:07 PDT
(In reply to comment #45)
> Created attachment 257041 [details]
> Archive of layout-test-results from ews105 for mac-mavericks-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

Forgetting a "return" in front of an expression will indeed
make things blow up.
Comment 47 Saam Barati 2015-07-18 21:52:13 PDT
Created attachment 257051 [details]
patch

lets try this again.
Comment 48 Saam Barati 2015-07-19 09:59:53 PDT
landed in:
http://trac.webkit.org/changeset/187012
Comment 49 Filip Pizlo 2015-07-22 20:23:15 PDT
The new tests added in this patch are incredibly slow.  Can you investigate making them run faster while still testing the same things?

For example, I'm not sure that running *everything* for 10000 times is particularly useful.  I think that most of the interesting semantic features are in bytecode and in runtime functions that are shared by all execution engines.  So, I think it would be worthwhile to reduce the running time, but write optimizing-compiler-specific tests that just cover the interesting things that happen in the optimizing JITs.
Comment 50 Saam Barati 2015-07-24 22:20:47 PDT
(In reply to comment #49)
> The new tests added in this patch are incredibly slow.  Can you investigate
> making them run faster while still testing the same things?
> 
> For example, I'm not sure that running *everything* for 10000 times is
> particularly useful.  I think that most of the interesting semantic features
> are in bytecode and in runtime functions that are shared by all execution
> engines.  So, I think it would be worthwhile to reduce the running time, but
> write optimizing-compiler-specific tests that just cover the interesting
> things that happen in the optimizing JITs.

I agree:
https://bugs.webkit.org/show_bug.cgi?id=147291
Comment 51 Saam Barati 2015-11-30 11:00:28 PST
*** Bug 74725 has been marked as a duplicate of this bug. ***