Bug 140435 – Implement ES6 Symbol

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

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)

[Yusuke Suzuki]

Created attachment 244603 [details]
prototype

Here's a prototype patch.

[Gavin Barraclough]

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?

[Gavin Barraclough]

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).

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 244858 [details]
Patch

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

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()).

[Yusuke Suzuki]

Created attachment 244859 [details]
prototype

Update the prototype

[Yusuke Suzuki]

Created attachment 244860 [details]
prototype

Update the prototype

[Yusuke Suzuki]

Created attachment 245298 [details]
Patch

[Yusuke Suzuki]

Created attachment 245299 [details]
Patch

[Yusuke Suzuki]

Created attachment 245300 [details]
Patch

[Build Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 245400 [details]
Patch

[Yusuke Suzuki]

Currently, only in OSX environment, StringImpl::createUnique crashes. I'll investigate it more.

[Yusuke Suzuki]

Created attachment 245401 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 245420 [details]
Patch

[Yusuke Suzuki]

Created attachment 245455 [details]
Patch

[Geoffrey Garen]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 245457 [details]
Patch

[Yusuke Suzuki]

(In reply to comment #28)
> Created attachment 245457 [details]
> Patch

Annotated with WTF_EXPORT_STRING_API.

[Yusuke Suzuki]

Created attachment 245458 [details]
Patch

[Geoffrey Garen]

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.

[Geoffrey Garen]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 245590 [details]
Patch

[Yusuke Suzuki]

Created attachment 245591 [details]
old benchmark result

[Yusuke Suzuki]

Created attachment 245592 [details]
latest benchmark result

[Yusuke Suzuki]

Created attachment 245594 [details]
Patch

[Yusuke Suzuki]

(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)

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 245602 [details]
Patch

[Yusuke Suzuki]

Created attachment 245636 [details]
Patch

[Geoffrey Garen]

These performance measurements are a bit noisy. I'm going to test performance on my machine when I have some time.

[Geoffrey Garen]

Created attachment 245728 [details]
Mac Pro run-jsc-benchmarks results

Performance looks OK.

[Geoffrey Garen]

Comment on attachment 245636 [details]
Patch

r=me

[Yusuke Suzuki]

(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+ :)

[WebKit Commit Bot]

Comment on attachment 245636 [details]
Patch

Clearing flags on attachment: 245636

Committed r179429: <http://trac.webkit.org/changeset/179429>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gavin Barraclough]

Very nice!

[Yusuke Suzuki]

(In reply to comment #51)
> Very nice!

Yay!

[Brent Fulgham]

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.

[Brent Fulgham]

Windows build fix landed in r179552. <http://trac.webkit.org/changeset/179552>

[Yusuke Suzuki]

(In reply to comment #54)
> Windows build fix landed in r179552.
> <http://trac.webkit.org/changeset/179552>

Oops! Thank you for your following fix.

[Filip Pizlo]

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?

[Yusuke Suzuki]

(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?