RESOLVED FIXED 140435
Implement ES6 Symbol
https://bugs.webkit.org/show_bug.cgi?id=140435
Summary Implement ES6 Symbol
Yusuke Suzuki
Reported 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)
Attachments
prototype (100.48 KB, patch)
2015-01-14 07:44 PST, Yusuke Suzuki
no flags
Patch (117.84 KB, patch)
2015-01-18 02:01 PST, Yusuke Suzuki
no flags
prototype (119.71 KB, patch)
2015-01-18 04:11 PST, Yusuke Suzuki
no flags
prototype (120.29 KB, patch)
2015-01-18 05:22 PST, Yusuke Suzuki
no flags
Patch (129.47 KB, patch)
2015-01-25 00:21 PST, Yusuke Suzuki
no flags
Patch (129.43 KB, patch)
2015-01-25 00:32 PST, Yusuke Suzuki
no flags
Patch (129.45 KB, patch)
2015-01-25 01:21 PST, Yusuke Suzuki
no flags
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
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
Patch (131.15 KB, patch)
2015-01-26 19:27 PST, Yusuke Suzuki
no flags
Patch (131.18 KB, patch)
2015-01-26 19:31 PST, Yusuke Suzuki
no flags
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
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
Patch (137.13 KB, patch)
2015-01-26 22:03 PST, Yusuke Suzuki
no flags
Patch (176.70 KB, patch)
2015-01-27 10:22 PST, Yusuke Suzuki
no flags
Patch (176.72 KB, patch)
2015-01-27 11:33 PST, Yusuke Suzuki
no flags
Patch (176.90 KB, patch)
2015-01-27 12:04 PST, Yusuke Suzuki
no flags
Patch (177.03 KB, patch)
2015-01-28 17:58 PST, Yusuke Suzuki
no flags
old benchmark result (43.12 KB, text/plain)
2015-01-28 17:59 PST, Yusuke Suzuki
no flags
latest benchmark result (43.62 KB, text/plain)
2015-01-28 17:59 PST, Yusuke Suzuki
no flags
Patch (176.95 KB, patch)
2015-01-28 18:07 PST, Yusuke Suzuki
no flags
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
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
Patch (177.16 KB, patch)
2015-01-28 19:38 PST, Yusuke Suzuki
no flags
Patch (177.15 KB, patch)
2015-01-29 11:24 PST, Yusuke Suzuki
no flags
Mac Pro run-jsc-benchmarks results (51.13 KB, text/plain)
2015-01-30 11:08 PST, Geoffrey Garen
no flags
Yusuke Suzuki
Comment 1 2015-01-14 07:44:51 PST
Created attachment 244603 [details] prototype Here's a prototype patch.
Gavin Barraclough
Comment 2 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?
Gavin Barraclough
Comment 3 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).
Yusuke Suzuki
Comment 4 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.
Yusuke Suzuki
Comment 5 2015-01-18 02:01:28 PST
WebKit Commit Bot
Comment 6 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.
Yusuke Suzuki
Comment 7 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()).
Yusuke Suzuki
Comment 8 2015-01-18 04:11:20 PST
Created attachment 244859 [details] prototype Update the prototype
Yusuke Suzuki
Comment 9 2015-01-18 05:22:26 PST
Created attachment 244860 [details] prototype Update the prototype
Yusuke Suzuki
Comment 10 2015-01-25 00:21:18 PST
Yusuke Suzuki
Comment 11 2015-01-25 00:32:47 PST
Yusuke Suzuki
Comment 12 2015-01-25 01:21:06 PST
Build Bot
Comment 13 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.
Build Bot
Comment 14 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.
Build Bot
Comment 15 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
Build Bot
Comment 16 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
Yusuke Suzuki
Comment 17 2015-01-26 19:27:21 PST
Yusuke Suzuki
Comment 18 2015-01-26 19:28:02 PST
Currently, only in OSX environment, StringImpl::createUnique crashes. I'll investigate it more.
Yusuke Suzuki
Comment 19 2015-01-26 19:31:35 PST
Build Bot
Comment 20 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
Build Bot
Comment 21 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
Build Bot
Comment 22 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
Build Bot
Comment 23 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
Yusuke Suzuki
Comment 24 2015-01-26 22:03:34 PST
Yusuke Suzuki
Comment 25 2015-01-27 10:22:11 PST
Geoffrey Garen
Comment 26 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.
Yusuke Suzuki
Comment 27 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.
Yusuke Suzuki
Comment 28 2015-01-27 11:33:29 PST
Yusuke Suzuki
Comment 29 2015-01-27 11:34:06 PST
(In reply to comment #28) > Created attachment 245457 [details] > Patch Annotated with WTF_EXPORT_STRING_API.
Yusuke Suzuki
Comment 30 2015-01-27 12:04:30 PST
Geoffrey Garen
Comment 31 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.
Geoffrey Garen
Comment 32 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
Yusuke Suzuki
Comment 33 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.
Yusuke Suzuki
Comment 34 2015-01-28 17:58:16 PST
Yusuke Suzuki
Comment 35 2015-01-28 17:59:21 PST
Created attachment 245591 [details] old benchmark result
Yusuke Suzuki
Comment 36 2015-01-28 17:59:43 PST
Created attachment 245592 [details] latest benchmark result
Yusuke Suzuki
Comment 37 2015-01-28 18:07:39 PST
Yusuke Suzuki
Comment 38 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)
Build Bot
Comment 39 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
Build Bot
Comment 40 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
Build Bot
Comment 41 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
Build Bot
Comment 42 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
Yusuke Suzuki
Comment 43 2015-01-28 19:38:51 PST
Yusuke Suzuki
Comment 44 2015-01-29 11:24:47 PST
Geoffrey Garen
Comment 45 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.
Geoffrey Garen
Comment 46 2015-01-30 11:08:57 PST
Created attachment 245728 [details] Mac Pro run-jsc-benchmarks results Performance looks OK.
Geoffrey Garen
Comment 47 2015-01-30 11:09:54 PST
Comment on attachment 245636 [details] Patch r=me
Yusuke Suzuki
Comment 48 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+ :)
WebKit Commit Bot
Comment 49 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>
WebKit Commit Bot
Comment 50 2015-01-30 17:24:18 PST
All reviewed patches have been landed. Closing bug.
Gavin Barraclough
Comment 51 2015-01-30 17:42:51 PST
Very nice!
Yusuke Suzuki
Comment 52 2015-01-30 17:47:03 PST
(In reply to comment #51) > Very nice! Yay!
Brent Fulgham
Comment 53 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.
Brent Fulgham
Comment 54 2015-02-03 10:17:57 PST
Windows build fix landed in r179552. <http://trac.webkit.org/changeset/179552>
Yusuke Suzuki
Comment 55 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.
Filip Pizlo
Comment 56 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?
Yusuke Suzuki
Comment 57 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?
Note You need to log in before you can comment on or make changes to this bug.