Bug 140435 - Implement ES6 Symbol
Summary: Implement ES6 Symbol
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 144402 142651
  Show dependency treegraph
 
Reported: 2015-01-14 02:38 PST by Yusuke Suzuki
Modified: 2015-04-29 11:49 PDT (History)
9 users (show)

See Also:


Attachments
prototype (100.48 KB, patch)
2015-01-14 07:44 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (117.84 KB, patch)
2015-01-18 02:01 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
prototype (119.71 KB, patch)
2015-01-18 04:11 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
prototype (120.29 KB, patch)
2015-01-18 05:22 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (129.47 KB, patch)
2015-01-25 00:21 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (129.43 KB, patch)
2015-01-25 00:32 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (129.45 KB, patch)
2015-01-25 01:21 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (245.61 KB, application/zip)
2015-01-25 02:34 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-mavericks (282.02 KB, application/zip)
2015-01-25 02:34 PST, Build Bot
no flags Details
Patch (131.15 KB, patch)
2015-01-26 19:27 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (131.18 KB, patch)
2015-01-26 19:31 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (533.58 KB, application/zip)
2015-01-26 20:45 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (658.50 KB, application/zip)
2015-01-26 20:53 PST, Build Bot
no flags Details
Patch (137.13 KB, patch)
2015-01-26 22:03 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (176.70 KB, patch)
2015-01-27 10:22 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (176.72 KB, patch)
2015-01-27 11:33 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (176.90 KB, patch)
2015-01-27 12:04 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (177.03 KB, patch)
2015-01-28 17:58 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
old benchmark result (43.12 KB, text/plain)
2015-01-28 17:59 PST, Yusuke Suzuki
no flags Details
latest benchmark result (43.62 KB, text/plain)
2015-01-28 17:59 PST, Yusuke Suzuki
no flags Details
Patch (176.95 KB, patch)
2015-01-28 18:07 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (521.85 KB, application/zip)
2015-01-28 19:21 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (641.91 KB, application/zip)
2015-01-28 19:28 PST, Build Bot
no flags Details
Patch (177.16 KB, patch)
2015-01-28 19:38 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (177.15 KB, patch)
2015-01-29 11:24 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Mac Pro run-jsc-benchmarks results (51.13 KB, text/plain)
2015-01-30 11:08 PST, Geoffrey Garen
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-01-14 02:38:00 PST
I'm planning to implement ES6 Symbol in JSC.
Here's a road map for this.

1. Introducing new primitive value, Symbol.
2. Introducing wrapper object SymbolObject.
3. Introducing Symbol related objects (SymbolPrototype, SymbolConstructor).
4. Replacing NameInstance / NamePrototype / NameConstructor with Symbols.
5. Introducing several builtin Symbols (e.g. Symbol.iterator)
Comment 1 Yusuke Suzuki 2015-01-14 07:44:51 PST
Created attachment 244603 [details]
prototype

Here's a prototype patch.
Comment 2 Gavin Barraclough 2015-01-15 11:17:50 PST
Comment on attachment 244603 [details]
prototype

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

> Source/JavaScriptCore/dfg/DFGOperations.cpp:-113
> -    if (isName(property)) {

Can this if block now be deleted? – looks like Symbols should be correctly handled below via toPropertyKey.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:-309
> -    if (isName(property))

ditto, do we still need this?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:-340
> -    if (isName(property))

ditto, do we still need this?
Comment 3 Gavin Barraclough 2015-01-15 11:20:21 PST
Sounds great!

From a quick look at the current patch, there are a bunch of places we used to have to explicitly check for private names, where it now looks like you'd just be able to delete the special case rather than converting to Symbol checks (with your toPropertyKey handling everything on a common path). That would be a nice cleanup (& also avoid adding extra branches).
Comment 4 Yusuke Suzuki 2015-01-15 13:21:08 PST
Comment on attachment 244603 [details]
prototype

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

Thank you for your review! Your suggestion is very reasonable. Dropping them simplifies the code :D

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:-113
>> -    if (isName(property)) {
> 
> Can this if block now be deleted? – looks like Symbols should be correctly handled below via toPropertyKey.

Right! It can be dropped :)
I'll update the patch.
Comment 5 Yusuke Suzuki 2015-01-18 02:01:28 PST
Created attachment 244858 [details]
Patch
Comment 6 WebKit Commit Bot 2015-01-18 02:03:40 PST
Attachment 244858 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SymbolConstructor.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSCJSValueInlines.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/Symbol.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolObject.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:118:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yusuke Suzuki 2015-01-18 02:12:55 PST
Comment on attachment 244858 [details]
Patch

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

Still super naive patch. Added a lot of implementations.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:-121
> -    }

Drop isName code :)

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:223
> +#define JSC_COMMON_PRIVATE_IDENTIFIERS_EACH_WELL_KNOWN_SYMBOL(macro) \

Introduce well known symbols in ES6 spec.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:832
> +        }

Added abstract equality check.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:860
> +        return asSymbol(v1)->privateName().uid() == asSymbol(v2)->privateName().uid();

The equality of Symbol primitives are measured by their uid address. Not cell address.
So Symbol::create(vm, uid) == Symbol::create(vm, uid);
It's useful to reconstruct Symbol primitives from PropertyTable.

> Source/JavaScriptCore/runtime/Symbol.cpp:47
> +    , m_privateName(string)

Folding the description string into private name uid.
As a result, PropertyTable still holds AtomicStringImpl* and we can generate Symbol primitive values from the PropertyTable's AtomicStringImpl.
It will be useful for implementing `Object.getOwnPropertySymbols`.

> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:55
> +    JSC_COMMON_PRIVATE_IDENTIFIERS_EACH_WELL_KNOWN_SYMBOL(INITIALIZE_WELL_KNOWN_SYMBOLS)

Defines well known symbols, like Symbol.iterator.

> Source/WTF/wtf/text/StringImpl.h:-381
> -    static Ref<StringImpl> createEmptyUnique()

Instead of StringImpl::createEmptyUnique, I've added createUnique functions that can generate unique strings that holds some contents.

> Source/WTF/wtf/text/StringImpl.h:412
> +    static unsigned flagIsUnique() { return s_hashFlagIsUnique; }

Add a new flag, isUnique, instead of checking empty static check (isEmptyUnique()).
Comment 8 Yusuke Suzuki 2015-01-18 04:11:20 PST
Created attachment 244859 [details]
prototype

Update the prototype
Comment 9 Yusuke Suzuki 2015-01-18 05:22:26 PST
Created attachment 244860 [details]
prototype

Update the prototype
Comment 10 Yusuke Suzuki 2015-01-25 00:21:18 PST
Created attachment 245298 [details]
Patch
Comment 11 Yusuke Suzuki 2015-01-25 00:32:47 PST
Created attachment 245299 [details]
Patch
Comment 12 Yusuke Suzuki 2015-01-25 01:21:06 PST
Created attachment 245300 [details]
Patch
Comment 13 Build Bot 2015-01-25 02:34:47 PST
Comment on attachment 245300 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 14 Build Bot 2015-01-25 02:34:49 PST
Comment on attachment 245300 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 15 Build Bot 2015-01-25 02:34:50 PST
Created attachment 245303 [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 16 Build Bot 2015-01-25 02:34:52 PST
Created attachment 245304 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 17 Yusuke Suzuki 2015-01-26 19:27:21 PST
Created attachment 245400 [details]
Patch
Comment 18 Yusuke Suzuki 2015-01-26 19:28:02 PST
Currently, only in OSX environment, StringImpl::createUnique crashes. I'll investigate it more.
Comment 19 Yusuke Suzuki 2015-01-26 19:31:35 PST
Created attachment 245401 [details]
Patch
Comment 20 Build Bot 2015-01-26 20:45:02 PST
Comment on attachment 245401 [details]
Patch

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

New failing tests:
js/dom/JSON-stringify.html
js/names.html
Comment 21 Build Bot 2015-01-26 20:45:05 PST
Created attachment 245407 [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 22 Build Bot 2015-01-26 20:53:27 PST
Comment on attachment 245401 [details]
Patch

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

New failing tests:
js/dom/JSON-stringify.html
js/names.html
Comment 23 Build Bot 2015-01-26 20:53:30 PST
Created attachment 245409 [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 24 Yusuke Suzuki 2015-01-26 22:03:34 PST
Created attachment 245420 [details]
Patch
Comment 25 Yusuke Suzuki 2015-01-27 10:22:11 PST
Created attachment 245455 [details]
Patch
Comment 26 Geoffrey Garen 2015-01-27 11:17:54 PST
Looks like the Windows .exp file might need some love:

            Creating library C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.lib and object C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.exp
     1>CommonIdentifiers.obj : error LNK2019: unresolved external symbol "public: static class WTF::Ref<class WTF::StringImpl> __cdecl WTF::StringImpl::createUnique(class WTF::PassRefPtr<class WTF::StringImpl>)" (?createUnique@StringImpl@WTF@@SA?AV?$Ref@VStringImpl@WTF@@@2@V?$PassRefPtr@VStringImpl@WTF@@@2@@Z) referenced in function "public: __thiscall JSC::BuiltinNames::BuiltinNames(class JSC::VM *,class JSC::CommonIdentifiers *)" (??0BuiltinNames@JSC@@QAE@PAVVM@1@PAVCommonIdentifiers@1@@Z)
     1>Symbol.obj : error LNK2001: unresolved external symbol "public: static class WTF::Ref<class WTF::StringImpl> __cdecl WTF::StringImpl::createUnique(class WTF::PassRefPtr<class WTF::StringImpl>)" (?createUnique@StringImpl@WTF@@SA?AV?$Ref@VStringImpl@WTF@@@2@V?$PassRefPtr@VStringImpl@WTF@@@2@@Z)
     1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\JavaScriptCore.dll : fatal error LNK1120: 1 unresolved externals
     1>Done Building Project "C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj" (Build target(s)) -- FAILED.
Comment 27 Yusuke Suzuki 2015-01-27 11:29:38 PST
(In reply to comment #26)
> Looks like the Windows .exp file might need some love:
> 
>             Creating library
> C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.lib
> and object
> C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.exp
>      1>CommonIdentifiers.obj : error LNK2019: unresolved external symbol
> "public: static class WTF::Ref<class WTF::StringImpl> __cdecl
> WTF::StringImpl::createUnique(class WTF::PassRefPtr<class WTF::StringImpl>)"
> (?createUnique@StringImpl@WTF@@SA?AV?$Ref@VStringImpl@WTF@@@2@V?$PassRefPtr@V
> StringImpl@WTF@@@2@@Z) referenced in function "public: __thiscall
> JSC::BuiltinNames::BuiltinNames(class JSC::VM *,class JSC::CommonIdentifiers
> *)" (??0BuiltinNames@JSC@@QAE@PAVVM@1@PAVCommonIdentifiers@1@@Z)
>      1>Symbol.obj : error LNK2001: unresolved external symbol "public:
> static class WTF::Ref<class WTF::StringImpl> __cdecl
> WTF::StringImpl::createUnique(class WTF::PassRefPtr<class WTF::StringImpl>)"
> (?createUnique@StringImpl@WTF@@SA?AV?$Ref@VStringImpl@WTF@@@2@V?$PassRefPtr@V
> StringImpl@WTF@@@2@@Z)
>     
> 1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\JavaScriptCore.
> dll : fatal error LNK1120: 1 unresolved externals
>      1>Done Building Project
> "C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.
> vcxproj\JavaScriptCore.vcxproj" (Build target(s)) -- FAILED.

Oops. Thank you!
Maybe, exporting these functions is needed.
Comment 28 Yusuke Suzuki 2015-01-27 11:33:29 PST
Created attachment 245457 [details]
Patch
Comment 29 Yusuke Suzuki 2015-01-27 11:34:06 PST
(In reply to comment #28)
> Created attachment 245457 [details]
> Patch

Annotated with WTF_EXPORT_STRING_API.
Comment 30 Yusuke Suzuki 2015-01-27 12:04:30 PST
Created attachment 245458 [details]
Patch
Comment 31 Geoffrey Garen 2015-01-27 14:58:04 PST
Comment on attachment 245458 [details]
Patch

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

How did you test the performance of this patch?

> Source/JavaScriptCore/runtime/MapData.h:138
> +    SymbolKeyedMap m_symbolKeyedTable;

If we guaranteed a 1-to-1 relationship between StrimImpl and Symbol, could we just store Symbols in the cell table?

> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:69
> +static EncodedJSValue JSC_HOST_CALL constructSymbol(ExecState* exec)
> +{
> +    return JSValue::encode(throwTypeError(exec));
> +}

Rather than implementing a constructor that throws, you should just return ConstructTypeNone fro getConstructData.

> Source/WTF/wtf/text/StringImpl.h:781
>      // The bottom 6 bits in the hash are flags.

This comment is wrong: It's the top seven bits.
Comment 32 Geoffrey Garen 2015-01-27 14:59:13 PST
I'd suggest running run-jsc-benchmarks, comparing with and without this patch, and posting the results.

The easiest way to do that is to build without your patch, archive the results, build with you patch, and then do this:

    run-jsc-benchmarks /path/to/archive/jsc /path/to/build/products/jsc
Comment 33 Yusuke Suzuki 2015-01-28 17:47:35 PST
Comment on attachment 245458 [details]
Patch

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

Thank you for your review!

>> Source/JavaScriptCore/runtime/MapData.h:138
>> +    SymbolKeyedMap m_symbolKeyedTable;
> 
> If we guaranteed a 1-to-1 relationship between StrimImpl and Symbol, could we just store Symbols in the cell table?

Yes. However, to maintain 1-to-1 relationship, we need to do
1. Storing <Symbol* | StringImpl*> as a PropertyTable's key instead of StringImpl* (to produce Symbol by Object.getOwnPropertySymbols)
2. GC need to mark Symbol* in PropertyTables

>> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:69
>> +}
> 
> Rather than implementing a constructor that throws, you should just return ConstructTypeNone fro getConstructData.

Thank you! I've changed.

>> Source/WTF/wtf/text/StringImpl.h:781
>>      // The bottom 6 bits in the hash are flags.
> 
> This comment is wrong: It's the top seven bits.

Oops! Fixed.
Comment 34 Yusuke Suzuki 2015-01-28 17:58:16 PST
Created attachment 245590 [details]
Patch
Comment 35 Yusuke Suzuki 2015-01-28 17:59:21 PST
Created attachment 245591 [details]
old benchmark result
Comment 36 Yusuke Suzuki 2015-01-28 17:59:43 PST
Created attachment 245592 [details]
latest benchmark result
Comment 37 Yusuke Suzuki 2015-01-28 18:07:39 PST
Created attachment 245594 [details]
Patch
Comment 38 Yusuke Suzuki 2015-01-28 18:14:05 PST
(In reply to comment #32)
> I'd suggest running run-jsc-benchmarks, comparing with and without this
> patch, and posting the results.
> 
> The easiest way to do that is to build without your patch, archive the
> results, build with you patch, and then do this:
> 
>     run-jsc-benchmarks /path/to/archive/jsc /path/to/build/products/jsc

Thanks. I attached the benchmark results and the updated patch.

The first one, "old benchmark result" is the benchmark result with the master and the previous patch.
In this result, I've found that there's incredible slow down in

    1. fold-get-by-id-to-multi-get-by-offset (definitely 2.8965x slower)
    2. fold-get-by-id-to-multi-get-by-offset-rare-int (definitely 2.7736x slower)

After investigating it, I guessed that it is raised because of the `toPropertyKey` function call instead of calling `toString(exec)->toIdentifier(exec)` directly.

So I updated the patch with adding ALWAYS_INLINE to toPropertyKey and took the benchmark. And the above results are improved.

    1. fold-get-by-id-to-multi-get-by-offset (definitely 1.0337x slower)
    2. fold-get-by-id-to-multi-get-by-offset-rare-int (might be 1.0444x slower)
Comment 39 Build Bot 2015-01-28 19:21:32 PST
Comment on attachment 245594 [details]
Patch

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

New failing tests:
js/symbol-object.html
Comment 40 Build Bot 2015-01-28 19:21:37 PST
Created attachment 245600 [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 41 Build Bot 2015-01-28 19:28:45 PST
Comment on attachment 245594 [details]
Patch

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

New failing tests:
js/symbol-object.html
Comment 42 Build Bot 2015-01-28 19:28:48 PST
Created attachment 245601 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 43 Yusuke Suzuki 2015-01-28 19:38:51 PST
Created attachment 245602 [details]
Patch
Comment 44 Yusuke Suzuki 2015-01-29 11:24:47 PST
Created attachment 245636 [details]
Patch
Comment 45 Geoffrey Garen 2015-01-29 12:01:45 PST
These performance measurements are a bit noisy. I'm going to test performance on my machine when I have some time.
Comment 46 Geoffrey Garen 2015-01-30 11:08:57 PST
Created attachment 245728 [details]
Mac Pro run-jsc-benchmarks results

Performance looks OK.
Comment 47 Geoffrey Garen 2015-01-30 11:09:54 PST
Comment on attachment 245636 [details]
Patch

r=me
Comment 48 Yusuke Suzuki 2015-01-30 16:32:17 PST
(In reply to comment #46)
> Created attachment 245728 [details]
> Mac Pro run-jsc-benchmarks results
> 
> Performance looks OK.

Thanks for measuring it and r+!
I've set cq+ :)
Comment 49 WebKit Commit Bot 2015-01-30 17:24:08 PST
Comment on attachment 245636 [details]
Patch

Clearing flags on attachment: 245636

Committed r179429: <http://trac.webkit.org/changeset/179429>
Comment 50 WebKit Commit Bot 2015-01-30 17:24:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 51 Gavin Barraclough 2015-01-30 17:42:51 PST
Very nice!
Comment 52 Yusuke Suzuki 2015-01-30 17:47:03 PST
(In reply to comment #51)
> Very nice!

Yay!
Comment 53 Brent Fulgham 2015-02-03 10:17:12 PST
Comment on attachment 245636 [details]
Patch

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

> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:857
> +    </ClInclude>

<ClCompile> must be matched with a </ClCompile>! This broke the project file format.
Comment 54 Brent Fulgham 2015-02-03 10:17:57 PST
Windows build fix landed in r179552. <http://trac.webkit.org/changeset/179552>
Comment 55 Yusuke Suzuki 2015-02-07 13:02:41 PST
(In reply to comment #54)
> Windows build fix landed in r179552.
> <http://trac.webkit.org/changeset/179552>

Oops! Thank you for your following fix.
Comment 56 Filip Pizlo 2015-04-29 11:23:41 PDT
Currently the DFG and FTL compilers make assumptions that if a user-visible cell is not a string then it must be an object and vice-versa.

Did you audit the code to make sure you fixed all of those cases?
Comment 57 Yusuke Suzuki 2015-04-29 11:30:49 PDT
(In reply to comment #56)
> Currently the DFG and FTL compilers make assumptions that if a user-visible
> cell is not a string then it must be an object and vice-versa.
> 
> Did you audit the code to make sure you fixed all of those cases?

Ooh, I think I fixed it in the separate patch.
https://bugs.webkit.org/show_bug.cgi?id=141640

Is there any cases I missed?