[DFG][FTL] Add NewSymbol
Created attachment 357116 [details] WIP
Created attachment 357123 [details] Patch
Comment on attachment 357123 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357123&action=review > Source/JavaScriptCore/ChangeLog:9 > + NewSymbol code faster. Rather than that, this patch intends to offer SpecSymbol type information into DFG's Do you think it could be worth inlining the allocation for this in another patch? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9662 > + // Known String. You should assert instead of comment
Comment on attachment 357123 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357123&action=review > Source/JavaScriptCore/ChangeLog:14 > + itself does not perform any type checks. Since `NewSymbol` will not exit except for OOM, we can just use ToString It’s worth saying this is because ToString performs effects but NewSymbol doesn’t. Also, I don’t think we will ever OOM here, we will just crash
Comment on attachment 357123 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357123&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + NewSymbol code faster. Rather than that, this patch intends to offer SpecSymbol type information into DFG's > > Do you think it could be worth inlining the allocation for this in another patch? One problem is that every Symbol has an unique WTF::SymbolImpl, which is allocated from bmalloc heap. While we can allocate Symbol JSCell in the inline code, we still need to call C runtime to allocate this SymbolImpl. If it still offers a perf win, it would be worth doing since Symbol creation would be more frequent after class private fields come. >> Source/JavaScriptCore/ChangeLog:14 >> + itself does not perform any type checks. Since `NewSymbol` will not exit except for OOM, we can just use ToString > > It’s worth saying this is because ToString performs effects but NewSymbol doesn’t. Also, I don’t think we will ever OOM here, we will just crash Sounds nice. Fixed. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9662 >> + // Known String. > > You should assert instead of comment Nice, fixed.
Committed r239142: <https://trac.webkit.org/changeset/239142>
<rdar://problem/46683296>
(In reply to Saam Barati from comment #3) > Comment on attachment 357123 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357123&action=review > > > Source/JavaScriptCore/ChangeLog:9 > > + NewSymbol code faster. Rather than that, this patch intends to offer SpecSymbol type information into DFG's > > Do you think it could be worth inlining the allocation for this in another > patch? After investigating, inlining allocation does not improve Symbol constructor calls much. Actually, the large part of the time is consumed by HashTable, which is used to ensure one-on-one correspondence betweeen Symbol* and WTF::SymbolImpl.
(In reply to Yusuke Suzuki from comment #8) > (In reply to Saam Barati from comment #3) > > Comment on attachment 357123 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=357123&action=review > > > > > Source/JavaScriptCore/ChangeLog:9 > > > + NewSymbol code faster. Rather than that, this patch intends to offer SpecSymbol type information into DFG's > > > > Do you think it could be worth inlining the allocation for this in another > > patch? > > After investigating, inlining allocation does not improve Symbol constructor > calls much. Actually, the large part of the time is consumed by HashTable, > which is used to ensure one-on-one correspondence betweeen Symbol* and > WTF::SymbolImpl. Makes sense. Thanks for investigating. If we ever found a use case where this is super hot, we could inline cache Symbol(), e.g, a program like this: function foo() { return Symbol(“const string”) } Where foo is frequently called. We could cache the underlying SymbolImpl
(In reply to Saam Barati from comment #9) > > Makes sense. Thanks for investigating. > > If we ever found a use case where this is super hot, we could inline cache > Symbol(), e.g, a program like this: > function foo() { return Symbol(“const string”) } > Where foo is frequently called. We could cache the underlying SymbolImpl The issue is that, everytime foo is called, it generates a new unique Symbol with unique SymbolImpl. So we cannot perform IC here. One fancy plan is that, embedding SymbolImpl into Symbol*. And Symbol* won't be collected while SymbolImpl's ref count is larger than one (this could be implemented by marking constraints). But the problem is, one SymbolImpl generated by VM cannot be used by the other VM. So, if we would like to do this optimization, I think we need per-process VM heap.
Makes sense. I clearly forgot that we have the 1:1 mapping of SymbolImpl to Symbol*