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)
Created attachment 244603 [details] prototype Here's a prototype patch.
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?
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 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.
Created attachment 244858 [details] Patch
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 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()).
Created attachment 244859 [details] prototype Update the prototype
Created attachment 244860 [details] prototype Update the prototype
Created attachment 245298 [details] Patch
Created attachment 245299 [details] Patch
Created attachment 245300 [details] Patch
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 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.
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
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
Created attachment 245400 [details] Patch
Currently, only in OSX environment, StringImpl::createUnique crashes. I'll investigate it more.
Created attachment 245401 [details] Patch
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
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 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
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
Created attachment 245420 [details] Patch
Created attachment 245455 [details] Patch
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.
(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.
Created attachment 245457 [details] Patch
(In reply to comment #28) > Created attachment 245457 [details] > Patch Annotated with WTF_EXPORT_STRING_API.
Created attachment 245458 [details] Patch
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.
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 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.
Created attachment 245590 [details] Patch
Created attachment 245591 [details] old benchmark result
Created attachment 245592 [details] latest benchmark result
Created attachment 245594 [details] Patch
(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 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
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 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
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
Created attachment 245602 [details] Patch
Created attachment 245636 [details] Patch
These performance measurements are a bit noisy. I'm going to test performance on my machine when I have some time.
Created attachment 245728 [details] Mac Pro run-jsc-benchmarks results Performance looks OK.
Comment on attachment 245636 [details] Patch r=me
(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 on attachment 245636 [details] Patch Clearing flags on attachment: 245636 Committed r179429: <http://trac.webkit.org/changeset/179429>
All reviewed patches have been landed. Closing bug.
Very nice!
(In reply to comment #51) > Very nice! Yay!
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.
Windows build fix landed in r179552. <http://trac.webkit.org/changeset/179552>
(In reply to comment #54) > Windows build fix landed in r179552. > <http://trac.webkit.org/changeset/179552> Oops! Thank you for your following fix.
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?
(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?